From 4819b230b0f758c6e59acf4c2209973afd1e2208 Mon Sep 17 00:00:00 2001 From: Jakub Turek Date: Mon, 12 Jun 2023 20:59:07 +0200 Subject: [PATCH 1/9] Fix iOS video_player package crash --- .../video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m b/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m index ba45d4d3ea0..4dac305cb55 100644 --- a/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m +++ b/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m @@ -530,6 +530,7 @@ - (void)disposeSansEventChannel { [currentItem removeObserver:self forKeyPath:@"playbackLikelyToKeepUp"]; [currentItem removeObserver:self forKeyPath:@"playbackBufferEmpty"]; [currentItem removeObserver:self forKeyPath:@"playbackBufferFull"]; + [self.player removeObserver:self forKeyPath:@"rate"]; [self.player replaceCurrentItemWithPlayerItem:nil]; [[NSNotificationCenter defaultCenter] removeObserver:self]; From 3a841821fc1c7dfa00d28d6c8e794e01d874c801 Mon Sep 17 00:00:00 2001 From: Jakub Turek Date: Wed, 14 Jun 2023 12:59:40 +0200 Subject: [PATCH 2/9] Prevent a crash on hot restart due to player being disposed twice --- .../ios/Classes/FLTVideoPlayerPlugin.m | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m b/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m index 4dac305cb55..ed2afa4cac5 100644 --- a/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m +++ b/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m @@ -533,6 +533,8 @@ - (void)disposeSansEventChannel { [self.player removeObserver:self forKeyPath:@"rate"]; [self.player replaceCurrentItemWithPlayerItem:nil]; + _player = nil; + [[NSNotificationCenter defaultCenter] removeObserver:self]; } From cf05e818e1e19810badf8140c40926ab9658e721 Mon Sep 17 00:00:00 2001 From: Jakub Turek Date: Fri, 16 Jun 2023 16:41:43 +0200 Subject: [PATCH 3/9] Add short-circuit for dispose method, do not clean up the _player --- .../ios/Classes/FLTVideoPlayerPlugin.m | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m b/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m index ed2afa4cac5..a9d37c2ce59 100644 --- a/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m +++ b/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m @@ -519,6 +519,14 @@ - (FlutterError *_Nullable)onListenWithArguments:(id _Nullable)arguments /// is useful for the case where the Engine is in the process of deconstruction /// so the channel is going to die or is already dead. - (void)disposeSansEventChannel { + // This check prevents the crash caused by removing the KVO observers twice. + // When performing a Hot Restart, the leftover players are disposed directly + // and also receive onTextureUnregistered: callback leading to possible + // over-release. + if (_disposed) { + return; + } + _disposed = YES; [_playerLayer removeFromSuperlayer]; [_displayLink invalidate]; @@ -533,8 +541,6 @@ - (void)disposeSansEventChannel { [self.player removeObserver:self forKeyPath:@"rate"]; [self.player replaceCurrentItemWithPlayerItem:nil]; - _player = nil; - [[NSNotificationCenter defaultCenter] removeObserver:self]; } From 2ba77520ba1d8bd3ac497d6f243b80793d1be478 Mon Sep 17 00:00:00 2001 From: Jakub Turek Date: Fri, 14 Jul 2023 12:38:26 +0200 Subject: [PATCH 4/9] Update the commend according to PR --- .../ios/Classes/FLTVideoPlayerPlugin.m | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m b/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m index a9d37c2ce59..73d9ea503b7 100644 --- a/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m +++ b/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m @@ -521,8 +521,8 @@ - (FlutterError *_Nullable)onListenWithArguments:(id _Nullable)arguments - (void)disposeSansEventChannel { // This check prevents the crash caused by removing the KVO observers twice. // When performing a Hot Restart, the leftover players are disposed directly - // and also receive onTextureUnregistered: callback leading to possible - // over-release. + // by [FLTVideoPlayerPlugin initialize:] method and also receive + // onTextureUnregistered: callback leading to possible over-release. if (_disposed) { return; } From 4b934f007fe18ce5cb288233d94897acb1b7b84a Mon Sep 17 00:00:00 2001 From: Jakub Turek Date: Fri, 14 Jul 2023 12:47:35 +0200 Subject: [PATCH 5/9] Bump version, update changelog --- packages/video_player/video_player_avfoundation/CHANGELOG.md | 5 +++++ packages/video_player/video_player_avfoundation/pubspec.yaml | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/video_player/video_player_avfoundation/CHANGELOG.md b/packages/video_player/video_player_avfoundation/CHANGELOG.md index e8642b6e04a..bf8136a277c 100644 --- a/packages/video_player/video_player_avfoundation/CHANGELOG.md +++ b/packages/video_player/video_player_avfoundation/CHANGELOG.md @@ -1,3 +1,8 @@ +## 2.4.8 + +* Fixes the iOS crash when using multiple players on the same screen. + See: https://github.com/flutter/flutter/issues/124937 + ## 2.4.7 * Updates minimum supported SDK version to Flutter 3.3/Dart 2.18. diff --git a/packages/video_player/video_player_avfoundation/pubspec.yaml b/packages/video_player/video_player_avfoundation/pubspec.yaml index a0c2f4ebae8..5c833930572 100644 --- a/packages/video_player/video_player_avfoundation/pubspec.yaml +++ b/packages/video_player/video_player_avfoundation/pubspec.yaml @@ -2,7 +2,7 @@ name: video_player_avfoundation description: iOS implementation of the video_player plugin. repository: https://github.com/flutter/packages/tree/main/packages/video_player/video_player_avfoundation issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22 -version: 2.4.7 +version: 2.4.8 environment: sdk: ">=2.18.0 <4.0.0" From cb08b43583d08cdcaa00281784c5d945edeb0abe Mon Sep 17 00:00:00 2001 From: Jakub Turek Date: Fri, 14 Jul 2023 18:19:35 +0200 Subject: [PATCH 6/9] Add tests for FLTVideoPlayer deallocation scenario --- .../ios/RunnerTests/VideoPlayerTests.m | 126 ++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m b/packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m index a9d7eac073d..0eb7f0e5c30 100644 --- a/packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m +++ b/packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m @@ -14,6 +14,8 @@ @interface FLTVideoPlayer : NSObject @property(readonly, nonatomic) AVPlayer *player; @property(readonly, nonatomic) AVPlayerLayer *playerLayer; @property(readonly, nonatomic) int64_t position; + +- (void)onTextureUnregistered:(NSObject *)texture; @end @interface FLTVideoPlayerPlugin (Test) @@ -396,6 +398,112 @@ - (void)testSeekToleranceWhenSeekingToEnd { return initializationEvent; } +// Checks whether [AVPlayer rate] KVO observations are correctly detached. +// - https://github.com/flutter/flutter/issues/124937 +// +// Failing to de-register results in a crash in [AVPlayer willChangeValueForKey:]. +- (void)testDoesNotCrashOnRateObservationAfterDisposal { + NSObject *registry = + (NSObject *)[[UIApplication sharedApplication] delegate]; + NSObject *registrar = + [registry registrarForPlugin:@"testDoesNotCrashOnRateObservationAfterDisposal"]; + + AVPlayer *avPlayer = nil; + __weak FLTVideoPlayer *player = nil; + + // Autoreleasepool is needed to simulate conditions of FLTVideoPlayer deallocation. + @autoreleasepool { + FLTVideoPlayerPlugin *videoPlayerPlugin = + (FLTVideoPlayerPlugin *)[[FLTVideoPlayerPlugin alloc] initWithRegistrar:registrar]; + + FlutterError *error; + [videoPlayerPlugin initialize:&error]; + XCTAssertNil(error); + + FLTCreateMessage *create = [FLTCreateMessage + makeWithAsset:nil + uri:@"https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4" + packageName:nil + formatHint:nil + httpHeaders:@{}]; + FLTTextureMessage *textureMessage = [videoPlayerPlugin create:create error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(textureMessage); + + player = videoPlayerPlugin.playersByTextureId[textureMessage.textureId]; + XCTAssertNotNil(player); + avPlayer = player.player; + + [videoPlayerPlugin dispose:textureMessage error:&error]; + XCTAssertNil(error); + } + + // [FLTVideoPlayerPlugin dispose:error:] selector is dispatching the [FLTVideoPlayer dispose] call + // with a 1-second delay keeping a strong reference to the player. The polling ensures the player + // was truly deallocated. + [self + waitForCondition:^BOOL() { + return player == nil; + } + withTimeout:10.0]; + + [avPlayer willChangeValueForKey:@"rate"]; // No assertions needed. Lack of crash is a success. +} + +// During the hot reload: +// 1. `[FLTVideoPlayer onTextureUnregistered:]` gets called. +// 2. `[FLTVideoPlayerPlugin initialize:]` gets called. +// +// Both of these methods dispatch [FLTVideoPlayer dispose] on the main thread +// leading to a possible crash when de-registering observers twice. +- (void)testHotReloadDoesNotCrash { + NSObject *registry = + (NSObject *)[[UIApplication sharedApplication] delegate]; + NSObject *registrar = + [registry registrarForPlugin:@"testHotReloadDoesNotCrash"]; + + __weak FLTVideoPlayer *player = nil; + + // Autoreleasepool is needed to simulate conditions of FLTVideoPlayer deallocation. + @autoreleasepool { + FLTVideoPlayerPlugin *videoPlayerPlugin = + (FLTVideoPlayerPlugin *)[[FLTVideoPlayerPlugin alloc] initWithRegistrar:registrar]; + + FlutterError *error; + [videoPlayerPlugin initialize:&error]; + XCTAssertNil(error); + + FLTCreateMessage *create = [FLTCreateMessage + makeWithAsset:nil + uri:@"https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4" + packageName:nil + formatHint:nil + httpHeaders:@{}]; + FLTTextureMessage *textureMessage = [videoPlayerPlugin create:create error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(textureMessage); + + player = videoPlayerPlugin.playersByTextureId[textureMessage.textureId]; + XCTAssertNotNil(player); + + [player onTextureUnregistered:nil]; + XCTAssertNil(error); + + [videoPlayerPlugin initialize:&error]; + XCTAssertNil(error); + } + + // [FLTVideoPlayerPlugin dispose:error:] selector is dispatching the [FLTVideoPlayer dispose] call + // with a 1-second delay keeping a strong reference to the player. The polling ensures the player + // was truly deallocated. + // + [self + waitForCondition:^BOOL() { + return player == nil; + } + withTimeout:10.0]; // No assertions needed. Lack of crash is a success. +} + - (void)validateTransformFixForOrientation:(UIImageOrientation)orientation { AVAssetTrack *track = [[FakeAVAssetTrack alloc] initWithOrientation:orientation]; CGAffineTransform t = FLTGetStandardizedTransformForTrack(track); @@ -439,4 +547,22 @@ - (void)validateTransformFixForOrientation:(UIImageOrientation)orientation { XCTAssertEqual(t.ty, expectY); } +- (void)waitForCondition:(BOOL (^)(void))condition withTimeout:(NSTimeInterval)timeout { + NSDate *end = [[NSDate date] dateByAddingTimeInterval:timeout]; + + do { + if (condition()) { + return; + } + + [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.002]]; + } while ([[NSDate date] timeIntervalSinceNow] < [end timeIntervalSinceNow]); + + if (condition()) { + return; + } + + XCTFail(@"Failed to fullfil the condition before timeout %f was exceeded", timeout); +} + @end From 84b0e24341b12228052bc901d278d3d7b0725fe9 Mon Sep 17 00:00:00 2001 From: Jakub Turek Date: Fri, 14 Jul 2023 18:21:57 +0200 Subject: [PATCH 7/9] Format the code --- .../example/ios/RunnerTests/VideoPlayerTests.m | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m b/packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m index 0eb7f0e5c30..c79441a9335 100644 --- a/packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m +++ b/packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m @@ -496,7 +496,6 @@ - (void)testHotReloadDoesNotCrash { // [FLTVideoPlayerPlugin dispose:error:] selector is dispatching the [FLTVideoPlayer dispose] call // with a 1-second delay keeping a strong reference to the player. The polling ensures the player // was truly deallocated. - // [self waitForCondition:^BOOL() { return player == nil; From 2e250836835ea3afe8b020decd3cd50a108cd8e7 Mon Sep 17 00:00:00 2001 From: Jakub Turek Date: Fri, 14 Jul 2023 22:02:56 +0200 Subject: [PATCH 8/9] Update code according to CR --- .../ios/RunnerTests/VideoPlayerTests.m | 37 +++++-------------- .../ios/Classes/FLTVideoPlayerPlugin.m | 6 +-- 2 files changed, 12 insertions(+), 31 deletions(-) diff --git a/packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m b/packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m index c79441a9335..969deaa4591 100644 --- a/packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m +++ b/packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m @@ -441,11 +441,10 @@ - (void)testDoesNotCrashOnRateObservationAfterDisposal { // [FLTVideoPlayerPlugin dispose:error:] selector is dispatching the [FLTVideoPlayer dispose] call // with a 1-second delay keeping a strong reference to the player. The polling ensures the player // was truly deallocated. - [self - waitForCondition:^BOOL() { - return player == nil; - } - withTimeout:10.0]; + [self expectationForPredicate:[NSPredicate predicateWithFormat:@"self != nil"] + evaluatedWithObject:player + handler:nil]; + [self waitForExpectationsWithTimeout:10.0 handler:nil]; [avPlayer willChangeValueForKey:@"rate"]; // No assertions needed. Lack of crash is a success. } @@ -496,11 +495,11 @@ - (void)testHotReloadDoesNotCrash { // [FLTVideoPlayerPlugin dispose:error:] selector is dispatching the [FLTVideoPlayer dispose] call // with a 1-second delay keeping a strong reference to the player. The polling ensures the player // was truly deallocated. - [self - waitForCondition:^BOOL() { - return player == nil; - } - withTimeout:10.0]; // No assertions needed. Lack of crash is a success. + [self expectationForPredicate:[NSPredicate predicateWithFormat:@"self != nil"] + evaluatedWithObject:player + handler:nil]; + [self waitForExpectationsWithTimeout:10.0 + handler:nil]; // No assertions needed. Lack of crash is a success. } - (void)validateTransformFixForOrientation:(UIImageOrientation)orientation { @@ -546,22 +545,4 @@ - (void)validateTransformFixForOrientation:(UIImageOrientation)orientation { XCTAssertEqual(t.ty, expectY); } -- (void)waitForCondition:(BOOL (^)(void))condition withTimeout:(NSTimeInterval)timeout { - NSDate *end = [[NSDate date] dateByAddingTimeInterval:timeout]; - - do { - if (condition()) { - return; - } - - [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:0.002]]; - } while ([[NSDate date] timeIntervalSinceNow] < [end timeIntervalSinceNow]); - - if (condition()) { - return; - } - - XCTFail(@"Failed to fullfil the condition before timeout %f was exceeded", timeout); -} - @end diff --git a/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m b/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m index 73d9ea503b7..f995ae87708 100644 --- a/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m +++ b/packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m @@ -520,9 +520,9 @@ - (FlutterError *_Nullable)onListenWithArguments:(id _Nullable)arguments /// so the channel is going to die or is already dead. - (void)disposeSansEventChannel { // This check prevents the crash caused by removing the KVO observers twice. - // When performing a Hot Restart, the leftover players are disposed directly - // by [FLTVideoPlayerPlugin initialize:] method and also receive - // onTextureUnregistered: callback leading to possible over-release. + // When performing a Hot Restart, the leftover players are disposed once directly + // by [FLTVideoPlayerPlugin initialize:] method and then disposed again by + // [FLTVideoPlayer onTextureUnregistered:] call leading to possible over-release. if (_disposed) { return; } From efd2567589cf92a2efe41d007b6a806c21945bdb Mon Sep 17 00:00:00 2001 From: Jakub Turek Date: Tue, 18 Jul 2023 11:17:04 +0200 Subject: [PATCH 9/9] Bump package version --- packages/video_player/video_player_avfoundation/pubspec.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/video_player/video_player_avfoundation/pubspec.yaml b/packages/video_player/video_player_avfoundation/pubspec.yaml index 5c833930572..606c587a41b 100644 --- a/packages/video_player/video_player_avfoundation/pubspec.yaml +++ b/packages/video_player/video_player_avfoundation/pubspec.yaml @@ -2,7 +2,7 @@ name: video_player_avfoundation description: iOS implementation of the video_player plugin. repository: https://github.com/flutter/packages/tree/main/packages/video_player/video_player_avfoundation issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22 -version: 2.4.8 +version: 2.4.9 environment: sdk: ">=2.18.0 <4.0.0"