Repository navigation
Conversation
github-actions
Bot
added
platform-ios
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the PointerDataDispatcher and SmoothPointerDataDispatcher classes, routing pointer events directly through the Animator and RuntimeController. It also removes FlutterMetalLayer in favor of standard CAMetalLayer on iOS, refactoring VsyncWaiterIOS to manage frame pacing via a layout trampoline view, and removes secondary vsync callbacks from VsyncWaiter. The review feedback identifies three critical issues: a potential data race on in_await_ in VsyncWaiter::FireCallback, an unsafe UIKit call on a background thread during VsyncWaiterIOS destruction, and a data race on the onFrameSubmitted property in FlutterPlatformViewsController due to its nonatomic declaration.
| VsyncWaiterIOS::~VsyncWaiterIOS() { | ||
| // This way, we will get no more callbacks from the display link that holds a weak (non-nilling) | ||
| // reference to this C++ object. | ||
| [client_ invalidate]; | ||
| [layout_trampoline_view_ removeFromSuperview]; | ||
| } |
There was a problem hiding this comment.
Safe UIKit Call on Destruction\n\nVsyncWaiterIOS::~VsyncWaiterIOS() calls [layout_trampoline_view_ removeFromSuperview]. Since VsyncWaiter is ref-counted via std::shared_ptr, if the last reference to VsyncWaiterIOS is released on a background thread (e.g., inside a background task or callback), the destructor will execute on that background thread. Calling UIKit methods like removeFromSuperview on a background thread is unsafe and can lead to crashes or undefined behavior.\n\nWe should defensively dispatch the removeFromSuperview call to the main thread if the destructor is executed on a background thread.
VsyncWaiterIOS::~VsyncWaiterIOS() {\n // This way, we will get no more callbacks from the display link that holds a weak (non-nilling)\n // reference to this C++ object.\n [client_ invalidate];\n if (layout_trampoline_view_) {\n UIView* view = layout_trampoline_view_;\n if ([NSThread isMainThread]) {\n [view removeFromSuperview];\n } else {\n dispatch_async(dispatch_get_main_queue(), ^{\n [view removeFromSuperview];\n });\n }\n }\n}
There was a problem hiding this comment.
I don't know why are you shouting but VsyncWaiterIOS should definitely be not released on background thread.
| @property(nonatomic, weak) UIViewController<FlutterViewResponder>* _Nullable flutterViewController; | ||
|
|
||
| /// Called from main thread when a frame has been submitted. | ||
| @property(readwrite, nonatomic) void (^onFrameSubmitted)(void); |
There was a problem hiding this comment.
Thread Safety of onFrameSubmitted Property\n\nThe onFrameSubmitted property is declared as nonatomic in FlutterPlatformViewsController.h:\nobjc\n@property(nonatomic, weak) UIViewController<FlutterViewResponder>* _Nullable flutterViewController;\n\n/// Called from main thread when a frame has been submitted.\n@property(readwrite, nonatomic) void (^onFrameSubmitted)(void);\n\nHowever, submitFrame: is called on the raster thread, where self.onFrameSubmitted is read:\nobjc\n if (self.onFrameSubmitted != nil) {\n\nSince the property is written on the main thread and read on the raster thread, declaring it as nonatomic introduces a data race. In Objective-C, accessing a nonatomic property concurrently from multiple threads is unsafe and can lead to undefined behavior or crashes.\n\n### Solution\nWe should declare the onFrameSubmitted property as copy (which is atomic by default) to ensure thread-safe accessors are generated by the compiler.
@property(readwrite, copy) void (^onFrameSubmitted)(void);
|
This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Fixes #189373
This PR depends on #191444 (for now squashed as first commit )
Removes
FlutterMetalLayerswitching back toCAMetalLayer, but ensures that the content is always present with transaction. Reduces the touch to preset latency by another frame matching UIKit.Background:
FlutterMetalLayerwas a wrong solution for a misunderstood problem: We were struggling to getCAMetaLayerwithpresentsWithTransaction=YESpresent from raster thread. Calling[drawable present]either from raster thread or through a hop to main thread resulted in very bad frame pacing. As a workaroundFlutterMetalLayerwas implemented, mirroring the API forCAMetalLayer, it worked through manually setting theIOSurfaceas layer content.FlutterMetalLayerfixed the frame pacing issue completely, but introduced another frame of latency (on top of the one caused by smooth event dispatcher). When investigating this latency I realized that the root cause was that because of asynchronous rendering (raster thread) we submit the content after the CA commit for current frame. ForFlutterMetalLayerthis results in the content being rendered in next frame (extra latency), forCAMetalLayerthis causes dropped frames.CA Commit is a UIKit phase that happens after all other phases (event dispatch, display link callback, layout, draw). The solution to fix both the latency and restore CAMetalLayer is to delay the CA commit until raster thread is done. This is done by holding the main thread at the latest possible time (see the changes in
vsync_waiter_ios.mm).Pre-launch Checklist
///).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-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.