From 02ffec575955781ad281ff0378b86e23a61d2eb8 Mon Sep 17 00:00:00 2001 From: Jenn Magder Date: Fri, 12 Apr 2024 20:50:14 -0700 Subject: [PATCH 1/2] Migrate vsync_waiter_ios to ARC --- shell/platform/darwin/ios/BUILD.gn | 6 +- .../framework/Source/VsyncWaiterIosTest.mm | 16 +++++ .../ios/framework/Source/vsync_waiter_ios.h | 7 +-- .../ios/framework/Source/vsync_waiter_ios.mm | 62 ++++++++----------- 4 files changed, 47 insertions(+), 44 deletions(-) diff --git a/shell/platform/darwin/ios/BUILD.gn b/shell/platform/darwin/ios/BUILD.gn index ca9dfb7453cd1..5f97a319f1620 100644 --- a/shell/platform/darwin/ios/BUILD.gn +++ b/shell/platform/darwin/ios/BUILD.gn @@ -82,6 +82,8 @@ source_set("flutter_framework_source_arc") { "framework/Source/KeyCodeMap_Internal.h", "framework/Source/UIViewController+FlutterScreenAndSceneIfLoaded.h", "framework/Source/UIViewController+FlutterScreenAndSceneIfLoaded.mm", + "framework/Source/vsync_waiter_ios.h", + "framework/Source/vsync_waiter_ios.mm", ] frameworks = [ @@ -90,8 +92,10 @@ source_set("flutter_framework_source_arc") { ] deps += [ + "//flutter/common:common", "//flutter/lib/ui", "//flutter/runtime", + "//flutter/shell/common", "//flutter/shell/platform/embedder:embedder_as_internal_library", ] } @@ -149,8 +153,6 @@ source_set("flutter_framework_source") { "framework/Source/platform_message_response_darwin.mm", "framework/Source/profiler_metrics_ios.h", "framework/Source/profiler_metrics_ios.mm", - "framework/Source/vsync_waiter_ios.h", - "framework/Source/vsync_waiter_ios.mm", "ios_context.h", "ios_context.mm", "ios_context_metal_impeller.h", diff --git a/shell/platform/darwin/ios/framework/Source/VsyncWaiterIosTest.mm b/shell/platform/darwin/ios/framework/Source/VsyncWaiterIosTest.mm index c4a98258b8fbb..cbe58384f6fdf 100644 --- a/shell/platform/darwin/ios/framework/Source/VsyncWaiterIosTest.mm +++ b/shell/platform/darwin/ios/framework/Source/VsyncWaiterIosTest.mm @@ -127,4 +127,20 @@ - (void)testAwaitAndPauseWillWorkCorrectly { XCTAssertTrue(link.isPaused); } +- (void)testReleasesLinkOnInvalidation { + __weak CADisplayLink* weakLink; + @autoreleasepool { + auto thread_task_runner = CreateNewThread("VsyncWaiterIosTest"); + VSyncClient* vsyncClient = [[VSyncClient alloc] + initWithTaskRunner:thread_task_runner + callback:[](std::unique_ptr recorder) {}]; + + weakLink = [vsyncClient getDisplayLink]; + XCTAssertNotNil(weakLink); + [vsyncClient invalidate]; + } + // VSyncClient has released the CADisplayLink. + XCTAssertNil(weakLink); +} + @end diff --git a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h index 36a608db7deca..3ff53c7cc05f5 100644 --- a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h +++ b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h @@ -8,8 +8,6 @@ #include #include "flutter/fml/macros.h" -#include "flutter/fml/memory/weak_ptr.h" -#include "flutter/fml/platform/darwin/scoped_nsobject.h" #include "flutter/shell/common/variable_refresh_rate_reporter.h" #include "flutter/shell/common/vsync_waiter.h" @@ -73,15 +71,12 @@ class VsyncWaiterIOS final : public VsyncWaiter, public VariableRefreshRateRepor // |VariableRefreshRateReporter| double GetRefreshRate() const override; - // Made public for testing. - fml::scoped_nsobject GetVsyncClient() const; - // |VsyncWaiter| // Made public for testing. void AwaitVSync() override; private: - fml::scoped_nsobject client_; + VSyncClient* client_; double max_refresh_rate_; FML_DISALLOW_COPY_AND_ASSIGN(VsyncWaiterIOS); diff --git a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm index 5171bf9153981..e85d585fe68e3 100644 --- a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm +++ b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm @@ -27,41 +27,36 @@ const fml::TimePoint target_time = recorder->GetVsyncTargetTime(); FireCallback(start_time, target_time, true); }; - client_ = - fml::scoped_nsobject{[[VSyncClient alloc] initWithTaskRunner:task_runners_.GetUITaskRunner() - callback:callback]}; + client_ = [[VSyncClient alloc] initWithTaskRunner:task_runners_.GetUITaskRunner() + callback:callback]; max_refresh_rate_ = [DisplayLinkManager displayRefreshRate]; } 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_.get() invalidate]; + [client_ invalidate]; } void VsyncWaiterIOS::AwaitVSync() { double new_max_refresh_rate = [DisplayLinkManager displayRefreshRate]; if (fabs(new_max_refresh_rate - max_refresh_rate_) > kRefreshRateDiffToIgnore) { max_refresh_rate_ = new_max_refresh_rate; - [client_.get() setMaxRefreshRate:max_refresh_rate_]; + [client_ setMaxRefreshRate:max_refresh_rate_]; } - [client_.get() await]; + [client_ await]; } // |VariableRefreshRateReporter| double VsyncWaiterIOS::GetRefreshRate() const { - return [client_.get() getRefreshRate]; -} - -fml::scoped_nsobject VsyncWaiterIOS::GetVsyncClient() const { - return client_; + return [client_ getRefreshRate]; } } // namespace flutter @implementation VSyncClient { flutter::VsyncWaiter::Callback callback_; - fml::scoped_nsobject display_link_; + CADisplayLink* _displayLink; double current_refresh_rate_; } @@ -73,17 +68,15 @@ - (instancetype)initWithTaskRunner:(fml::RefPtr)task_runner current_refresh_rate_ = [DisplayLinkManager displayRefreshRate]; _allowPauseAfterVsync = YES; callback_ = std::move(callback); - display_link_ = fml::scoped_nsobject { - [[CADisplayLink displayLinkWithTarget:self selector:@selector(onDisplayLink:)] retain] - }; - display_link_.get().paused = YES; + _displayLink = [CADisplayLink displayLinkWithTarget:self selector:@selector(onDisplayLink:)]; + _displayLink.paused = YES; [self setMaxRefreshRate:[DisplayLinkManager displayRefreshRate]]; - task_runner->PostTask([client = [self retain]]() { - [client->display_link_.get() addToRunLoop:[NSRunLoop currentRunLoop] - forMode:NSRunLoopCommonModes]; - [client release]; + // Strongly retain the the captured link until it is added to the runloop. + CADisplayLink* localDisplayLink = _displayLink; + task_runner->PostTask([localDisplayLink]() { + [localDisplayLink addToRunLoop:[NSRunLoop currentRunLoop] forMode:NSRunLoopCommonModes]; }); } @@ -97,19 +90,19 @@ - (void)setMaxRefreshRate:(double)refreshRate { double maxFrameRate = fmax(refreshRate, 60); double minFrameRate = fmax(maxFrameRate / 2, 60); if (@available(iOS 15.0, *)) { - display_link_.get().preferredFrameRateRange = + _displayLink.preferredFrameRateRange = CAFrameRateRangeMake(minFrameRate, maxFrameRate, maxFrameRate); } else { - display_link_.get().preferredFramesPerSecond = maxFrameRate; + _displayLink.preferredFramesPerSecond = maxFrameRate; } } - (void)await { - display_link_.get().paused = NO; + _displayLink.paused = NO; } - (void)pause { - display_link_.get().paused = YES; + _displayLink.paused = YES; } - (void)onDisplayLink:(CADisplayLink*)link { @@ -130,19 +123,18 @@ - (void)onDisplayLink:(CADisplayLink*)link { recorder->RecordVsync(frame_start_time, frame_target_time); if (_allowPauseAfterVsync) { - display_link_.get().paused = YES; + link.paused = YES; } callback_(std::move(recorder)); } - (void)invalidate { - [display_link_.get() invalidate]; + [_displayLink invalidate]; + _displayLink = nil; // Break retain cycle. } - (void)dealloc { - [self invalidate]; - - [super dealloc]; + [_displayLink invalidate]; } - (double)getRefreshRate { @@ -150,7 +142,7 @@ - (double)getRefreshRate { } - (CADisplayLink*)getDisplayLink { - return display_link_.get(); + return _displayLink; } @end @@ -158,12 +150,10 @@ - (CADisplayLink*)getDisplayLink { @implementation DisplayLinkManager + (double)displayRefreshRate { - fml::scoped_nsobject display_link = fml::scoped_nsobject { - [[CADisplayLink displayLinkWithTarget:[[[DisplayLinkManager alloc] init] autorelease] - selector:@selector(onDisplayLink:)] retain] - }; - display_link.get().paused = YES; - auto preferredFPS = display_link.get().preferredFramesPerSecond; + CADisplayLink* displayLink = [CADisplayLink displayLinkWithTarget:[[[self class] alloc] init] + selector:@selector(onDisplayLink:)]; + displayLink.paused = YES; + auto preferredFPS = displayLink.preferredFramesPerSecond; // From Docs: // The default value for preferredFramesPerSecond is 0. When this value is 0, the preferred From 7fcab7707cb905ab1138879c6aeb7aaf7deb577a Mon Sep 17 00:00:00 2001 From: Jenn Magder Date: Mon, 15 Apr 2024 12:26:52 -0700 Subject: [PATCH 2/2] Review edits --- .../ios/framework/Source/FlutterMetalLayer.mm | 6 +-- .../framework/Source/FlutterViewController.mm | 2 +- .../ios/framework/Source/vsync_waiter_ios.h | 7 ++-- .../ios/framework/Source/vsync_waiter_ios.mm | 37 ++++++++----------- 4 files changed, 24 insertions(+), 28 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterMetalLayer.mm b/shell/platform/darwin/ios/framework/Source/FlutterMetalLayer.mm index b5c95966e291a..83d248f7cdcb1 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterMetalLayer.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterMetalLayer.mm @@ -176,7 +176,7 @@ - (instancetype)init { _availableTextures = [[NSMutableSet alloc] init]; _displayLink = [CADisplayLink displayLinkWithTarget:self selector:@selector(onDisplayLink:)]; - [self setMaxRefreshRate:[DisplayLinkManager displayRefreshRate] forceMax:NO]; + [self setMaxRefreshRate:DisplayLinkManager.displayRefreshRate forceMax:NO]; [_displayLink addToRunLoop:[NSRunLoop currentRunLoop] forMode:NSRunLoopCommonModes]; [[NSNotificationCenter defaultCenter] addObserver:self selector:@selector(didEnterBackground:) @@ -214,7 +214,7 @@ - (void)onDisplayLink:(CADisplayLink*)link { if (_displayLinkPauseCountdown == 3) { _displayLink.paused = YES; if (_displayLinkForcedMaxRate) { - [self setMaxRefreshRate:[DisplayLinkManager displayRefreshRate] forceMax:NO]; + [self setMaxRefreshRate:DisplayLinkManager.displayRefreshRate forceMax:NO]; _displayLinkForcedMaxRate = NO; } } else { @@ -395,7 +395,7 @@ - (void)presentOnMainThread:(FlutterTexture*)texture { _didSetContentsDuringThisDisplayLinkPeriod = YES; } else if (!_displayLinkForcedMaxRate) { _displayLinkForcedMaxRate = YES; - [self setMaxRefreshRate:[DisplayLinkManager displayRefreshRate] forceMax:YES]; + [self setMaxRefreshRate:DisplayLinkManager.displayRefreshRate forceMax:YES]; } } diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index 1134b735c68fe..4bf66d112e62b 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -1321,7 +1321,7 @@ - (void)createTouchRateCorrectionVSyncClientIfNeeded { return; } - double displayRefreshRate = [DisplayLinkManager displayRefreshRate]; + double displayRefreshRate = DisplayLinkManager.displayRefreshRate; const double epsilon = 0.1; if (displayRefreshRate < 60.0 + epsilon) { // displayRefreshRate <= 60.0 diff --git a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h index 3ff53c7cc05f5..c4395f47a6982 100644 --- a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h +++ b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h @@ -29,7 +29,7 @@ /// /// @return The refresh rate in frames per second. /// -+ (double)displayRefreshRate; +@property(class, nonatomic, readonly) double displayRefreshRate; @end @@ -52,10 +52,11 @@ - (void)pause; +//------------------------------------------------------------------------------ +/// @brief Call invalidate before releasing this object to remove from runloops. +/// - (void)invalidate; -- (double)getRefreshRate; - - (void)setMaxRefreshRate:(double)refreshRate; @end diff --git a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm index e85d585fe68e3..2544144e66bcf 100644 --- a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm +++ b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm @@ -15,6 +15,10 @@ #include "flutter/fml/memory/task_runner_checker.h" #include "flutter/fml/trace_event.h" +@interface VSyncClient () +@property(nonatomic, assign, readonly) double refreshRate; +@end + // When calculating refresh rate diffrence, anything within 0.1 fps is ignored. const static double kRefreshRateDiffToIgnore = 0.1; @@ -29,7 +33,7 @@ }; client_ = [[VSyncClient alloc] initWithTaskRunner:task_runners_.GetUITaskRunner() callback:callback]; - max_refresh_rate_ = [DisplayLinkManager displayRefreshRate]; + max_refresh_rate_ = DisplayLinkManager.displayRefreshRate; } VsyncWaiterIOS::~VsyncWaiterIOS() { @@ -39,7 +43,7 @@ } void VsyncWaiterIOS::AwaitVSync() { - double new_max_refresh_rate = [DisplayLinkManager displayRefreshRate]; + double new_max_refresh_rate = DisplayLinkManager.displayRefreshRate; if (fabs(new_max_refresh_rate - max_refresh_rate_) > kRefreshRateDiffToIgnore) { max_refresh_rate_ = new_max_refresh_rate; [client_ setMaxRefreshRate:max_refresh_rate_]; @@ -49,15 +53,14 @@ // |VariableRefreshRateReporter| double VsyncWaiterIOS::GetRefreshRate() const { - return [client_ getRefreshRate]; + return client_.refreshRate; } } // namespace flutter @implementation VSyncClient { - flutter::VsyncWaiter::Callback callback_; + flutter::VsyncWaiter::Callback _callback; CADisplayLink* _displayLink; - double current_refresh_rate_; } - (instancetype)initWithTaskRunner:(fml::RefPtr)task_runner @@ -65,18 +68,18 @@ - (instancetype)initWithTaskRunner:(fml::RefPtr)task_runner self = [super init]; if (self) { - current_refresh_rate_ = [DisplayLinkManager displayRefreshRate]; + _refreshRate = DisplayLinkManager.displayRefreshRate; _allowPauseAfterVsync = YES; - callback_ = std::move(callback); + _callback = std::move(callback); _displayLink = [CADisplayLink displayLinkWithTarget:self selector:@selector(onDisplayLink:)]; _displayLink.paused = YES; - [self setMaxRefreshRate:[DisplayLinkManager displayRefreshRate]]; + [self setMaxRefreshRate:DisplayLinkManager.displayRefreshRate]; // Strongly retain the the captured link until it is added to the runloop. CADisplayLink* localDisplayLink = _displayLink; task_runner->PostTask([localDisplayLink]() { - [localDisplayLink addToRunLoop:[NSRunLoop currentRunLoop] forMode:NSRunLoopCommonModes]; + [localDisplayLink addToRunLoop:NSRunLoop.currentRunLoop forMode:NSRunLoopCommonModes]; }); } @@ -119,13 +122,13 @@ - (void)onDisplayLink:(CADisplayLink*)link { std::unique_ptr recorder = std::make_unique(); - current_refresh_rate_ = round(1 / (frame_target_time - frame_start_time).ToSecondsF()); + _refreshRate = round(1 / (frame_target_time - frame_start_time).ToSecondsF()); recorder->RecordVsync(frame_start_time, frame_target_time); if (_allowPauseAfterVsync) { link.paused = YES; } - callback_(std::move(recorder)); + _callback(std::move(recorder)); } - (void)invalidate { @@ -133,14 +136,6 @@ - (void)invalidate { _displayLink = nil; // Break retain cycle. } -- (void)dealloc { - [_displayLink invalidate]; -} - -- (double)getRefreshRate { - return current_refresh_rate_; -} - - (CADisplayLink*)getDisplayLink { return _displayLink; } @@ -164,7 +159,7 @@ + (double)displayRefreshRate { return preferredFPS; } - return [UIScreen mainScreen].maximumFramesPerSecond; + return UIScreen.mainScreen.maximumFramesPerSecond; } - (void)onDisplayLink:(CADisplayLink*)link { @@ -172,7 +167,7 @@ - (void)onDisplayLink:(CADisplayLink*)link { } + (BOOL)maxRefreshRateEnabledOnIPhone { - return [[[NSBundle mainBundle] objectForInfoDictionaryKey:@"CADisableMinimumFrameDurationOnPhone"] + return [[NSBundle.mainBundle objectForInfoDictionaryKey:@"CADisableMinimumFrameDurationOnPhone"] boolValue]; }