-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Platform view blur clipping - Rounded Rect (iOS) #177551
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
Platform view blur clipping - Rounded Rect (iOS) #177551
Conversation
|
Tested this fix in our production app (heavy usage of blurred overlays over platform views). The clipping works perfectly, but we're seeing significant performance impact:
Potential optimisations:
Not my domain, so not 100% sure. Happy to provide profiling data or test optimisation approaches if there's interest. Test setup: Flutter 3.35.7 baseline vs custom build with this PR, iPhone 16 Pro (iOS 18.6.2) |
|
Thank you for testing it out, I had worked on this based off a simple example, happy to iterate to make this performant enough to land.
This should be happening in the baseline as well, as my PR is modifying the existing blur's corner radius rather than blurring in a new way. So it's weird if you're seeing that as a difference between baseline and this PR. Would be great if you can provide profiling data for both baseline on main and against this PR. Do you have multiple rounded blurs on a platform view or is it like a scrollable of rounded blurs on the platform view? Trying to setup a reproducible sample |
|
@LukeMoody01 diving a bit more into the bottlenecks and optimisations you have mentioned - any chance this was AI generated? Because this isn't really making sense to me based off of the code. For example
As I understand I did have some stray logs I had added in this PR that I have now removed, can you try rerunning your performance benchmark? I think the perf hit just came from those rather than anything else |
Bottlenecks were indeed AI-generated based on your git diff, engine code isn’t my usual domain, so I’d take those suggestions with a healthy grain of salt. The performance data, however, is real, measured directly from a production-level test. I can share that along with a small reproducible repo sometime in the coming weeks. Re your question, we have roughly four rounded-blur overlays layered on top of a scrollable platform view (a webview in this case). I will also retest now that you have removed the logs |
6a5d6b3 to
af18084
Compare
Please avoid these IMO. While its obviously helpful to test it out and share the performance you saw, if unfamiliar its better not to comment on bottlenecks and fixes rather than use AI and give wrong suggestions which ended up wasting my time trying to make sense of it |
gmackall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From android triage: just noting that the small android portion looks reasonable, to get it out of the android triage queue
|
I think through asserts we can add a log statement if a user tries to do something that's not been implemented linking to the existing issue , if that's acceptable |
|
Sorry for the delay. I will take a look at this one. |
|
@hellohuanlin any chance you might be able to take a look this week? |
| kBackdropFilter, | ||
| kBackdropClipRect, | ||
| kBackdropClipRRect, | ||
| kBackdropClipRse, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rse isn't a widely used acronym. I'd go with RSuperellipse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this change but right above it does say kClipRSE
| case MutatorType::kClipPath: | ||
| case MutatorType::kClipRRect: | ||
| case MutatorType::kClipRSE: | ||
| // TODO: Figure out if this matters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reminder on this before landing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this thing is irrelevant now.
IsClipType() was only used in one place earlier and that got deleted a while back in flutter/engine#53826
Currently looks like IsClipType() isn't referenced by anything. Im making this false since they're not clips on the embedded view, and just modifies the backdrop on it. But dont think it matters either way
| visualEffectView.frame = _frame; | ||
|
|
||
| visualEffectView.layer.cornerRadius = _cornerRadius; | ||
| visualEffectView.clipsToBounds = YES; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this cause regression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so
clipsToBounds just prevents subviews of visualEffectView from going outside its bounds, that seems safe currently
| UIVisualEffectView* visualEffectView = [[UIVisualEffectView alloc] | ||
| initWithEffect:[UIBlurEffect effectWithStyle:UIBlurEffectStyleLight]]; | ||
|
|
||
| // TODO: Technically we should be using the largest corner radius, but currently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be tracked by a github issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed as #179126
| [pendingClipRRects addObject:clip]; | ||
| } | ||
| case flutter::MutatorType::kBackdropClipRse: { | ||
| // TODO: Pending Implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed as #179125
Manual roll Flutter from d81baab to 57c3f8b (38 revisions) Manual roll requested by stuartmorgan@google.com flutter/flutter@d81baab...57c3f8b 2025-12-17 bruno.leroux@gmail.com Add FloatingActionButtonTheme (flutter/flutter#179736) 2025-12-17 engine-flutter-autoroll@skia.org Roll Skia from b1569739f431 to fb576bd6827a (1 revision) (flutter/flutter#179989) 2025-12-17 bruno.leroux@gmail.com Update more comments related to theme normalization (flutter/flutter#179682) 2025-12-17 engine-flutter-autoroll@skia.org Roll Skia from cce9b86bda7d to b1569739f431 (1 revision) (flutter/flutter#179979) 2025-12-17 jobguldemeester@gmail.com Adds property passthrough for CheckboxListTile, SwitchListTile and RadioListTile (flutter/flutter#178098) 2025-12-17 engine-flutter-autoroll@skia.org Roll Dart SDK from ca949b915846 to 3793f3d2d0c4 (2 revisions) (flutter/flutter#179973) 2025-12-17 engine-flutter-autoroll@skia.org Roll Skia from 318400199beb to cce9b86bda7d (1 revision) (flutter/flutter#179976) 2025-12-17 ahmedsameha1@gmail.com Make sure that a CupertinoTextFormFieldRow doesn't crash in 0x0 envir… (flutter/flutter#179932) 2025-12-17 ahmedsameha1@gmail.com Make sure that a CupertinoTabView doesn't crash in 0x0 environment (flutter/flutter#179845) 2025-12-17 ahmedsameha1@gmail.com Make sure that a CupertinoTextField doesn't crash in 0x0 environment (flutter/flutter#179865) 2025-12-17 ahmedsameha1@gmail.com Make sure that a CupertinoSwitch doesn't crash in 0x0 environment (flutter/flutter#179748) 2025-12-17 116356835+AbdeMohlbi@users.noreply.github.com Update `BuildContext` docs to make it easier to understand (flutter/flutter#178616) 2025-12-17 41930132+hellohuanlin@users.noreply.github.com [ios][pv] quick fix to enable and re-enable web view's gesture recognizer (flutter/flutter#179908) 2025-12-17 biggs0125@gmail.com Deduplicate wasm dry run entries in analytics. (flutter/flutter#179970) 2025-12-17 engine-flutter-autoroll@skia.org Roll Skia from 99899cbb415b to 318400199beb (1 revision) (flutter/flutter#179969) 2025-12-17 engine-flutter-autoroll@skia.org Roll Skia from 2ac4a8709bc9 to 99899cbb415b (1 revision) (flutter/flutter#179968) 2025-12-17 engine-flutter-autoroll@skia.org Roll Dart SDK from 95a92bc705d3 to ca949b915846 (6 revisions) (flutter/flutter#179967) 2025-12-17 biggs0125@gmail.com Add package info to wasm dry run events. (flutter/flutter#179826) 2025-12-17 matt.boetger@gmail.com Platform View Hide/Show Integration test (flutter/flutter#179902) 2025-12-16 engine-flutter-autoroll@skia.org Roll Skia from 61162d72343f to 2ac4a8709bc9 (14 revisions) (flutter/flutter#179961) 2025-12-16 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from 433KtmJvbMyaDMJvD... to fAoyBAT99XxwPE5hL... (flutter/flutter#179960) 2025-12-16 bkonyi@google.com [ Tool ] Fix update-packages not accounting for path dependencies (flutter/flutter#179951) 2025-12-16 45459898+RamonFarizel@users.noreply.github.com ListTile fix MinIntrinsicHeight calculation (flutter/flutter#179515) 2025-12-16 108678139+manu-sncf@users.noreply.github.com Fix pinned header in NestedScrollView (flutter/flutter#179210) 2025-12-16 bruno.leroux@gmail.com Update some comments related to theme normalization (flutter/flutter#179624) 2025-12-16 iliyazelenkog@gmail.com Add Cyrillic keyboard layout support for flutter_tools terminal commands (flutter/flutter#177855) 2025-12-16 lauren@selfisekai.rocks Minor fixes for libstdc++ 15 (flutter/flutter#178601) 2025-12-16 34465683+rkishan516@users.noreply.github.com Feat: Add top gap for cupertino sheet (flutter/flutter#171348) 2025-12-16 116356835+AbdeMohlbi@users.noreply.github.com Align `Build.API_LEVELS` usage in `FlutterImageDecoder.java` with existing usage (flutter/flutter#179868) 2025-12-16 41930132+hellohuanlin@users.noreply.github.com Revert "[ios][pv] accept/reject gesture based on hitTest (with new wi… (flutter/flutter#179895) 2025-12-16 codefu@google.com fix: line endings for flutter/dart/flutter-dev (flutter/flutter#179912) 2025-12-16 22373191+Hari-07@users.noreply.github.com Platform view blur clipping - Rounded Rect (iOS) (flutter/flutter#177551) 2025-12-16 engine-flutter-autoroll@skia.org Roll Dart SDK from 20d114f951db to 95a92bc705d3 (1 revision) (flutter/flutter#179909) 2025-12-16 34871572+gmackall@users.noreply.github.com [Reland] Unmodified android sdk bundle (flutter/flutter#179920) 2025-12-16 engine-flutter-autoroll@skia.org Roll Skia from 6903a4e65c3f to 61162d72343f (2 revisions) (flutter/flutter#179915) 2025-12-16 engine-flutter-autoroll@skia.org Roll Packages from 2cd921c to 57725eb (1 revision) (flutter/flutter#179942) 2025-12-16 45490440+shindonghwi@users.noreply.github.com Filter out FrameEvents/updateAcquireFence log spam from adb logcat (flutter/flutter#179884) 2025-12-16 30870216+gaaclarke@users.noreply.github.com `flutter update-packages --force-upgrade --update-hashes` (flutter/flutter#179950) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. ...
This PR aims to fix blurs on platform views not being clipped on ios. Currently the clips were not forwarded to the mutator stack and thus just ignored. The frame the blur is in was being used which would always behave like a clip rect ignoring any corner radii or clip paths etc
Now all rrect, rse and path clips are being forwarded to the mutator stack.
Currently only rounded rect has been implemented though, and even in that just a single uniform corner radii, which seems to be most of the use cases from users who have run into this issue.
Example:
Before:

After:

List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
#115926
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot 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.