Sitelet https://github.com/flutter/flutter/pull/191325
Skip to content

[Impeller] fix position of cached single glyph text shadows - #191325

Merged
auto-submit[bot] merged 8 commits into
flutter:masterfrom
flar:impeller-single-glyph-shadow-caching
Aug 21, 2026
Merged

[Impeller] fix position of cached single glyph text shadows#191325
auto-submit[bot] merged 8 commits into
flutter:masterfrom
flar:impeller-single-glyph-shadow-caching

Conversation

@flar

@flar flar commented Aug 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

The cache key for single-glyph TextFrame shadows did not include enough information to accurately resolve the operation with their cached shadows. This PR removes the special case for the single glyph case so all TextFrames will now be cached consistently and correctly.

Fixes: #191161

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on [Discord].

If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label Aug 18, 2026
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Aug 18, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors text rendering tests and optimizations in Impeller. It introduces a helper function RePositionTextFrame to shift text frames, updates MakeDefaultTextFrame to support custom text strings, adds a new unit test for single-glyph text with shadows, and simplifies fingerprint generation in Canvas::AttemptBlurredTextOptimization. Feedback suggests updating the original parameter in RePositionTextFrame to be a const reference to comply with the Google C++ Style Guide.

Comment thread engine/src/flutter/impeller/display_list/aiks_dl_text_unittests.cc Outdated
@flar
flar requested review from b-luk and gaaclarke August 18, 2026 23:39
@github-actions github-actions Bot added a: text input Entering text in a text field or keyboard related problems engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Aug 18, 2026
@flar

flar commented Aug 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

This change points out that the font and "single_glyph" entries in the TextShadowCacheKey are essentially redundant now. Fix it here, or in a separate PR to isolate any potential side effects?

gaaclarke
gaaclarke previously approved these changes Aug 18, 2026
View reviewed changes

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm! if any benchmarks regress we can reevaluate

@flar

flar commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

lgtm! if any benchmarks regress we can reevaluate

Thoughts on simplifying the Key struct here or in a follow-on PR? I'm leaning towards a follow-on since this particular fix should be safe to CP if we need to, but trying to simplify the Key struct, even if it is fairly obvious, could introduce another bug...

b-luk
b-luk previously approved these changes Aug 18, 2026
View reviewed changes
Comment thread engine/src/flutter/impeller/display_list/aiks_dl_text_unittests.cc Outdated
@b-luk

b-luk commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

lgtm! if any benchmarks regress we can reevaluate

Thoughts on simplifying the Key struct here or in a follow-on PR? I'm leaning towards a follow-on since this particular fix should be safe to CP if we need to, but trying to simplify the Key struct, even if it is fairly obvious, could introduce another bug...

If we want to CP it, we would need to CP #190681 first, right? Is that what we want to do?

@flar

flar commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

lgtm! if any benchmarks regress we can reevaluate

Thoughts on simplifying the Key struct here or in a follow-on PR? I'm leaning towards a follow-on since this particular fix should be safe to CP if we need to, but trying to simplify the Key struct, even if it is fairly obvious, could introduce another bug...

If we want to CP it, we would need to CP #190681 first, right? Is that what we want to do?

Good point. I don't think this is fixable without #190681, though. The whole idea of the fix is that it makes the lookup key more specific than just the basic glyph id info. So, if we CP, we have to CP a bunch of the current stuff.

@flar
flar dismissed stale reviews from b-luk and gaaclarke via 0a40a74 August 19, 2026 01:20
@flar

flar commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

I committed to the full fix with removing the redundant key elements.

@flar
flar requested review from b-luk and gaaclarke August 19, 2026 01:22
@flar flar changed the title [Impeller] fix position of cached single glyph text frames [Impeller] fix position of cached single glyph text shadows Aug 19, 2026
b-luk
b-luk requested changes Aug 19, 2026
View reviewed changes
Comment thread engine/src/flutter/impeller/display_list/canvas.cc
@flar
flar requested a review from b-luk August 20, 2026 04:40
b-luk
b-luk requested changes Aug 20, 2026
View reviewed changes

@b-luk b-luk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One cleanup comment about the unit test. Otherwise LGTM

Comment thread engine/src/flutter/impeller/display_list/aiks_dl_text_unittests.cc Outdated
@flar
flar requested a review from b-luk August 20, 2026 21:15
@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 21, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Aug 21, 2026
Merged via the queue into flutter:master with commit c3586de Aug 21, 2026
22 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 21, 2026
This was referenced Aug 21, 2026
Closed
Closed
Closed
Closed
Closed
Merged
auto-submit Bot pushed a commit to flutter/packages that referenced this pull request Aug 22, 2026
@engine-flutter-autoroll
Roll Flutter from c2437523d308 to 65c9a8dc60bc (195 revisions) (#12554)
9cb9cc4 
Roll Flutter from c2437523d308 to 65c9a8dc60bc (195 revisions)

flutter/flutter@c243752...65c9a8d

2026-08-22 engine-flutter-autoroll@skia.org Roll ICU from d578f2e8b7bd to 8cc91d9b6ab9 (1 revision) (flutter/flutter#191542)
2026-08-22 engine-flutter-autoroll@skia.org Roll Skia from 666a9b3d5cf0 to ad2106c0bb64 (3 revisions) (flutter/flutter#191527)
2026-08-22 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from KaOq3EE4qJ9fnaaaK... to 0iCv10IlKfiilEBOU... (flutter/flutter#191524)
2026-08-22 bkonyi@google.com [flutter_tools] Add tests for negative lookahead regex in test runner and batch entrypoints (flutter/flutter#191438)
2026-08-22 engine-flutter-autoroll@skia.org Roll Skia from 0c37868737fa to 666a9b3d5cf0 (2 revisions) (flutter/flutter#191518)
2026-08-21 10456171+caroqliu@users.noreply.github.com Revert "[input] Migrate fuchsia.ui.pointerinjector to TouchSource (#190855) (flutter/flutter#191509)
2026-08-21 30870216+gaaclarke@users.noreply.github.com Fixes windows gallery benchmarks by forcing mobile layout (flutter/flutter#191507)
2026-08-21 bkonyi@google.com [flutter_tools] Restrict WebAssetServer source resolution to source map extensions (flutter/flutter#191501)
2026-08-21 engine-flutter-autoroll@skia.org Roll Skia from f6900c5b8439 to 0c37868737fa (2 revisions) (flutter/flutter#191504)
2026-08-21 bkonyi@google.com Refactor `FlutterDevice.connect` and VM service discovery (flutter/flutter#191221)
2026-08-21 bkonyi@google.com [flutter_tools] Fix crash when migrating flow-style exclude lists in analysis_options.yaml (flutter/flutter#191269)
2026-08-21 269567208+reidbaker-agent@users.noreply.github.com [rules] Add packages/flutter_tools/gradle/AGENTS.md rules (flutter/flutter#191486)
2026-08-21 kevmoo@users.noreply.github.com [flutter_tools] refactor CLI argument architecture with typed option descriptors and bundles (PoC) (flutter/flutter#191018)
2026-08-21 bkonyi@google.com tools: Extract Dart SDK to temp directory before moving to final location (flutter/flutter#191263)
2026-08-21 1961493+harryterkelsen@users.noreply.github.com [web] Move CanvasKit fragment shader classes to canvaskit/fragment_shader.dart (flutter/flutter#191451)
2026-08-21 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from GCQlmt6h-esJsNubS... to ic6GjOSn-KN508XyK... (flutter/flutter#191485)
2026-08-21 bkonyi@google.com [Widget Preview] Isolate PageStorage scope in widget preview group expansion tile (flutter/flutter#191378)
2026-08-21 bkonyi@google.com [flutter_tools] Deprecate --build and --no-build flags on flutter run (flutter/flutter#191358)
2026-08-21 engine-flutter-autoroll@skia.org Roll Skia from 70988bed1b3b to f6900c5b8439 (2 revisions) (flutter/flutter#191481)
2026-08-21 engine-flutter-autoroll@skia.org Roll Packages from 1785501 to 252bb33 (6 revisions) (flutter/flutter#191480)
2026-08-21 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 20IJas24bZiNmCZTK... to GCQlmt6h-esJsNubS... (flutter/flutter#191415)
2026-08-21 engine-flutter-autoroll@skia.org Roll Skia from 2ba6971bd0d1 to 70988bed1b3b (2 revisions) (flutter/flutter#191476)
2026-08-21 engine-flutter-autoroll@skia.org Roll Skia from 1d5f72537ba6 to 2ba6971bd0d1 (2 revisions) (flutter/flutter#191473)
2026-08-21 engine-flutter-autoroll@skia.org Roll Skia from 2c25efd2e369 to 1d5f72537ba6 (1 revision) (flutter/flutter#191472)
2026-08-21 engine-flutter-autoroll@skia.org Roll Skia from 09b1b810850a to 2c25efd2e369 (9 revisions) (flutter/flutter#191470)
2026-08-21 flar@google.com [Impeller] fix position of cached single glyph text shadows (flutter/flutter#191325)
2026-08-21 engine-flutter-autoroll@skia.org Roll Skia from abdf8821f313 to 09b1b810850a (15 revisions) (flutter/flutter#191458)
2026-08-21 me@bnsaed.com Document that programmatic TextEditingController changes do not run input formatters (flutter/flutter#190166)
2026-08-21 30870216+gaaclarke@users.noreply.github.com Adds new gallery benchmarks to windows (skia and impeller) (flutter/flutter#191454)
2026-08-21 chris@bracken.jp iOS: Deprecate FlutterEngine.isGpuDisabled (flutter/flutter#191393)
2026-08-21 154381524+flutteractionsbot@users.noreply.github.com Revert: [web] Unskip decoration image lerp tests (flutter/flutter#191462)
2026-08-20 awolff@google.com android_hardware_smoke_test: Improve reliability (flutter/flutter#191374)
2026-08-20 77467499+wilyan09007@users.noreply.github.com Don't size or offset the Android platform view before it is laid out (flutter/flutter#190895)
2026-08-20 15619084+vashworth@users.noreply.github.com [iOS][add2app] Skip building SwiftPM plugins when generating CocoaPods artifacts (flutter/flutter#190736)
2026-08-20 33794642+FelixMittermeier@users.noreply.github.com Optimize JSONMessageCodec UTF-8 conversion (flutter/flutter#190529)
2026-08-20 bkonyi@google.com [FML] Replace deprecated wstring_convert with Win32 APIs (flutter/flutter#191394)
2026-08-20 victorsanniay@gmail.com SliverFillRemaining extends beyond viewport size when fillOverscroll is true (flutter/flutter#191236)
2026-08-20 bkonyi@google.com [flutter_tools] Update argParser usageLineLength when --wrap-column is passed (flutter/flutter#191264)
2026-08-20 bkonyi@google.com [flutter_tools] Fix UNC path resolution in depfile parsing on Windows (flutter/flutter#191265)
2026-08-20 1961493+harryterkelsen@users.noreply.github.com [web] Unskip decoration image lerp tests (flutter/flutter#191426)
2026-08-20 bkonyi@google.com Do not inject 'type' and 'method' into service extension responses (flutter/flutter#190946)
2026-08-20 bkonyi@google.com [flutter_tools] Fix crash in symbolize command on stream error (flutter/flutter#191273)
2026-08-20 bkonyi@google.com [flutter_tools] Prevent deletion of shared native asset hooks outputs as stale (flutter/flutter#191272)
2026-08-20 bkonyi@google.com [flutter_tools] Support package wildcard assets in app pubspec (flutter/flutter#191266)
2026-08-20 108678139+manu-sncf@users.noreply.github.com Add SliverClipRect and SliverClipRRect (flutter/flutter#179003)
2026-08-20 bkonyi@google.com [flutter_tools] Implement Diagnostics extension slice and doctor integration (flutter/flutter#191162)
...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@b-luk b-luk b-luk approved these changes
@gaaclarke gaaclarke Awaiting requested review from gaaclarke
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: text input Entering text in a text field or keyboard related problems CICD Run CI/CD e: impeller Impeller rendering backend issues and features requests engine flutter/engine related. See also e: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Impeller] A blurred TextStyle shadow detaches from its glyph when the same single-glyph run is painted more than once

3 participants

@flar @b-luk @gaaclarke