[video_player] On iOS deregister KVO observer on AVPlayer

Author: jmagman

packages/video_player/video_player/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 2.2.14
+
+* Remove KVO observer on AVPlayerItem on iOS.
+
 ## 2.2.13
 
 * Fixes persisting of hasError even after successful initialize.

packages/video_player/video_player/example/ios/RunnerTests/VideoPlayerTests.m

@@ -2,34 +2,37 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+@import AVFoundation;
 @import video_player;
 @import XCTest;
 
 #import <OCMock/OCMock.h>
 
+@interface FLTVideoPlayer : NSObject
+@property(readonly, nonatomic) AVPlayer *player;
+@end
+
+@interface FLTVideoPlayerPlugin (Test) <FLTVideoPlayerApi>
+@property(readonly, strong, nonatomic)
+    NSMutableDictionary<NSNumber *, FLTVideoPlayer *> *playersByTextureId;
+@end
+
 @interface VideoPlayerTests : XCTestCase
 @end
 
 @implementation VideoPlayerTests
 
-- (void)testPlugin {
-  FLTVideoPlayerPlugin *plugin = [[FLTVideoPlayerPlugin alloc] init];
-  XCTAssertNotNil(plugin);
-}
-
 - (void)testSeekToInvokesTextureFrameAvailableOnTextureRegistry {
   NSObject<FlutterTextureRegistry> *mockTextureRegistry =
       OCMProtocolMock(@protocol(FlutterTextureRegistry));
   NSObject<FlutterPluginRegistry> *registry =
       (NSObject<FlutterPluginRegistry> *)[[UIApplication sharedApplication] delegate];
   NSObject<FlutterPluginRegistrar> *registrar =
-      [registry registrarForPlugin:@"TEST_FLTVideoPlayerPlugin"];
+      [registry registrarForPlugin:@"SeekToInvokestextureFrameAvailable"];
   NSObject<FlutterPluginRegistrar> *partialRegistrar = OCMPartialMock(registrar);
   OCMStub([partialRegistrar textures]).andReturn(mockTextureRegistry);
-  [FLTVideoPlayerPlugin registerWithRegistrar:partialRegistrar];
-  FLTVideoPlayerPlugin<FLTVideoPlayerApi> *videoPlayerPlugin =
-      (FLTVideoPlayerPlugin<FLTVideoPlayerApi> *)[[FLTVideoPlayerPlugin alloc]
-          initWithRegistrar:partialRegistrar];
+  FLTVideoPlayerPlugin *videoPlayerPlugin =
+      (FLTVideoPlayerPlugin *)[[FLTVideoPlayerPlugin alloc] initWithRegistrar:partialRegistrar];
   FLTPositionMessage *message = [[FLTPositionMessage alloc] init];
   message.textureId = @101;
   message.position = @0;
@@ -38,4 +41,33 @@ - (void)testSeekToInvokesTextureFrameAvailableOnTextureRegistry {
   OCMVerify([mockTextureRegistry textureFrameAvailable:message.textureId.intValue]);
 }
 
+- (void)testDeregistersFromPlayer {
+  NSObject<FlutterPluginRegistry> *registry =
+      (NSObject<FlutterPluginRegistry> *)[[UIApplication sharedApplication] delegate];
+  NSObject<FlutterPluginRegistrar> *registrar =
+      [registry registrarForPlugin:@"testDeregistersFromPlayer"];
+  FLTVideoPlayerPlugin *videoPlayerPlugin =
+      (FLTVideoPlayerPlugin *)[[FLTVideoPlayerPlugin alloc] initWithRegistrar:registrar];
+
+  FlutterError *error;
+  [videoPlayerPlugin initialize:&error];
+  XCTAssertNil(error);
+
+  FLTCreateMessage *create = [[FLTCreateMessage alloc] init];
+  create.uri = @"https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4";
+  FLTTextureMessage *textureMessage = [videoPlayerPlugin create:create error:&error];
+  XCTAssertNil(error);
+  XCTAssertNotNil(textureMessage);
+  FLTVideoPlayer *player = videoPlayerPlugin.playersByTextureId[textureMessage.textureId];
+  XCTAssertNotNil(player);
+  AVPlayer *avPlayer = player.player;
+
+  [videoPlayerPlugin dispose:textureMessage error:&error];
+  XCTAssertEqual(videoPlayerPlugin.playersByTextureId.count, 0);
+  XCTAssertNil(error);
+
+  [self keyValueObservingExpectationForObject:avPlayer keyPath:@"currentItem" expectedValue:nil];
+  [self waitForExpectationsWithTimeout:1 handler:nil];
+}
+
 @end

packages/video_player/video_player/ios/Classes/FLTVideoPlayerPlugin.m

@@ -37,17 +37,13 @@ @interface FLTVideoPlayer : NSObject <FlutterTexture, FlutterStreamHandler>
 @property(nonatomic) FlutterEventChannel* eventChannel;
 @property(nonatomic) FlutterEventSink eventSink;
 @property(nonatomic) CGAffineTransform preferredTransform;
-@property(nonatomic, readonly) bool disposed;
-@property(nonatomic, readonly) bool isPlaying;
-@property(nonatomic) bool isLooping;
-@property(nonatomic, readonly) bool isInitialized;
+@property(nonatomic, readonly) BOOL disposed;
+@property(nonatomic, readonly) BOOL isPlaying;
+@property(nonatomic) BOOL isLooping;
+@property(nonatomic, readonly) BOOL isInitialized;
 - (instancetype)initWithURL:(NSURL*)url
                frameUpdater:(FLTFrameUpdater*)frameUpdater
                 httpHeaders:(NSDictionary<NSString*, NSString*>*)headers;
-- (void)play;
-- (void)pause;
-- (void)setIsLooping:(bool)isLooping;
-- (void)updatePlayingState;
 @end
 
 static void* timeRangeContext = &timeRangeContext;
@@ -114,22 +110,22 @@ - (void)itemDidPlayToEndTime:(NSNotification*)notification {
 
 const int64_t TIME_UNSET = -9223372036854775807;
 
-static inline int64_t FLTCMTimeToMillis(CMTime time) {
+NS_INLINE int64_t FLTCMTimeToMillis(CMTime time) {
   // When CMTIME_IS_INDEFINITE return a value that matches TIME_UNSET from ExoPlayer2 on Android.
   // Fixes https://github.com/flutter/flutter/issues/48670
   if (CMTIME_IS_INDEFINITE(time)) return TIME_UNSET;
   if (time.timescale == 0) return 0;
   return time.value * 1000 / time.timescale;
 }
 
-static inline CGFloat radiansToDegrees(CGFloat radians) {
+NS_INLINE CGFloat radiansToDegrees(CGFloat radians) {
   // Input range [-pi, pi] or [-180, 180]
   CGFloat degrees = GLKMathRadiansToDegrees((float)radians);
   if (degrees < 0) {
     // Convert -90 to 270 and -180 to 180
     return degrees + 360;
   }
-  // Output degrees in between [0, 360[
+  // Output degrees in between [0, 360]
   return degrees;
 };
 
@@ -217,9 +213,6 @@ - (CGAffineTransform)fixTransform:(AVAssetTrack*)videoTrack {
 - (instancetype)initWithPlayerItem:(AVPlayerItem*)item frameUpdater:(FLTFrameUpdater*)frameUpdater {
   self = [super init];
   NSAssert(self, @"super init cannot be nil");
-  _isInitialized = false;
-  _isPlaying = false;
-  _disposed = false;
 
   AVAsset* asset = [item asset];
   void (^assetCompletionHandler)(void) = ^{
@@ -352,7 +345,7 @@ - (void)setupEventSinkIfReadyToPlay {
       return;
     }
 
-    _isInitialized = true;
+    _isInitialized = YES;
     _eventSink(@{
       @"event" : @"initialized",
       @"duration" : @([self duration]),
@@ -363,12 +356,12 @@ - (void)setupEventSinkIfReadyToPlay {
 }
 
 - (void)play {
-  _isPlaying = true;
+  _isPlaying = YES;
   [self updatePlayingState];
 }
 
 - (void)pause {
-  _isPlaying = false;
+  _isPlaying = NO;
   [self updatePlayingState];
 }
 
@@ -389,7 +382,7 @@ - (void)seekTo:(int)location {
        toleranceAfter:kCMTimeZero];
 }
 
-- (void)setIsLooping:(bool)isLooping {
+- (void)setIsLooping:(BOOL)isLooping {
   _isLooping = isLooping;
 }
 
@@ -457,22 +450,18 @@ - (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 {
-  _disposed = true;
+  _disposed = YES;
   [_displayLink invalidate];
-  [[_player currentItem] removeObserver:self forKeyPath:@"status" context:statusContext];
-  [[_player currentItem] removeObserver:self
-                             forKeyPath:@"loadedTimeRanges"
-                                context:timeRangeContext];
-  [[_player currentItem] removeObserver:self
-                             forKeyPath:@"playbackLikelyToKeepUp"
-                                context:playbackLikelyToKeepUpContext];
-  [[_player currentItem] removeObserver:self
-                             forKeyPath:@"playbackBufferEmpty"
-                                context:playbackBufferEmptyContext];
-  [[_player currentItem] removeObserver:self
-                             forKeyPath:@"playbackBufferFull"
-                                context:playbackBufferFullContext];
-  [_player replaceCurrentItemWithPlayerItem:nil];
+  AVPlayerItem* currentItem = self.player.currentItem;
+  [currentItem removeObserver:self forKeyPath:@"status"];
+  [currentItem removeObserver:self forKeyPath:@"loadedTimeRanges"];
+  [currentItem removeObserver:self forKeyPath:@"presentationSize"];
+  [currentItem removeObserver:self forKeyPath:@"duration"];
+  [currentItem removeObserver:self forKeyPath:@"playbackLikelyToKeepUp"];
+  [currentItem removeObserver:self forKeyPath:@"playbackBufferEmpty"];
+  [currentItem removeObserver:self forKeyPath:@"playbackBufferFull"];
+
+  [self.player replaceCurrentItemWithPlayerItem:nil];
   [[NSNotificationCenter defaultCenter] removeObserver:self];
 }
 
@@ -486,7 +475,8 @@ - (void)dispose {
 @interface FLTVideoPlayerPlugin () <FLTVideoPlayerApi>
 @property(readonly, weak, nonatomic) NSObject<FlutterTextureRegistry>* registry;
 @property(readonly, weak, nonatomic) NSObject<FlutterBinaryMessenger>* messenger;
-@property(readonly, strong, nonatomic) NSMutableDictionary* players;
+@property(readonly, strong, nonatomic)
+    NSMutableDictionary<NSNumber*, FLTVideoPlayer*>* playersByTextureId;
 @property(readonly, strong, nonatomic) NSObject<FlutterPluginRegistrar>* registrar;
 @end
 
@@ -503,16 +493,13 @@ - (instancetype)initWithRegistrar:(NSObject<FlutterPluginRegistrar>*)registrar {
   _registry = [registrar textures];
   _messenger = [registrar messenger];
   _registrar = registrar;
-  _players = [NSMutableDictionary dictionaryWithCapacity:1];
+  _playersByTextureId = [NSMutableDictionary dictionaryWithCapacity:1];
   return self;
 }
 
 - (void)detachFromEngineForRegistrar:(NSObject<FlutterPluginRegistrar>*)registrar {
-  for (NSNumber* textureId in _players.allKeys) {
-    FLTVideoPlayer* player = _players[textureId];
-    [player disposeSansEventChannel];
-  }
-  [_players removeAllObjects];
+  [self.playersByTextureId.allValues makeObjectsPerformSelector:@selector(disposeSansEventChannel)];
+  [self.playersByTextureId removeAllObjects];
   // TODO(57151): This should be commented out when 57151's fix lands on stable.
   // This is the correct behavior we never did it in the past and the engine
   // doesn't currently support it.
@@ -521,15 +508,15 @@ - (void)detachFromEngineForRegistrar:(NSObject<FlutterPluginRegistrar>*)registra
 
 - (FLTTextureMessage*)onPlayerSetup:(FLTVideoPlayer*)player
                        frameUpdater:(FLTFrameUpdater*)frameUpdater {
-  int64_t textureId = [_registry registerTexture:player];
+  int64_t textureId = [self.registry registerTexture:player];
   frameUpdater.textureId = textureId;
   FlutterEventChannel* eventChannel = [FlutterEventChannel
       eventChannelWithName:[NSString stringWithFormat:@"flutter.io/videoPlayer/videoEvents%lld",
                                                       textureId]
            binaryMessenger:_messenger];
   [eventChannel setStreamHandler:player];
   player.eventChannel = eventChannel;
-  _players[@(textureId)] = player;
+  self.playersByTextureId[@(textureId)] = player;
   FLTTextureMessage* result = [[FLTTextureMessage alloc] init];
   result.textureId = @(textureId);
   return result;
@@ -539,11 +526,12 @@ - (void)initialize:(FlutterError* __autoreleasing*)error {
   // Allow audio playback when the Ring/Silent switch is set to silent
   [[AVAudioSession sharedInstance] setCategory:AVAudioSessionCategoryPlayback error:nil];
 
-  for (NSNumber* textureId in _players) {
-    [_registry unregisterTexture:[textureId unsignedIntegerValue]];
-    [_players[textureId] dispose];
-  }
-  [_players removeAllObjects];
+  [self.playersByTextureId
+      enumerateKeysAndObjectsUsingBlock:^(NSNumber* textureId, FLTVideoPlayer* player, BOOL* stop) {
+        [self.registry unregisterTexture:textureId.unsignedIntegerValue];
+        [player dispose];
+      }];
+  [self.playersByTextureId removeAllObjects];
 }
 
 - (FLTTextureMessage*)create:(FLTCreateMessage*)input error:(FlutterError**)error {
@@ -570,9 +558,9 @@ - (FLTTextureMessage*)create:(FLTCreateMessage*)input error:(FlutterError**)erro
 }
 
 - (void)dispose:(FLTTextureMessage*)input error:(FlutterError**)error {
-  FLTVideoPlayer* player = _players[input.textureId];
-  [_registry unregisterTexture:input.textureId.intValue];
-  [_players removeObjectForKey:input.textureId];
+  FLTVideoPlayer* player = self.playersByTextureId[input.textureId];
+  [self.registry unregisterTexture:input.textureId.intValue];
+  [self.playersByTextureId removeObjectForKey:input.textureId];
   // If the Flutter contains https://github.com/flutter/engine/pull/12695,
   // the `player` is disposed via `onTextureUnregistered` at the right time.
   // Without https://github.com/flutter/engine/pull/12695, there is no guarantee that the
@@ -592,46 +580,46 @@ - (void)dispose:(FLTTextureMessage*)input error:(FlutterError**)error {
 }
 
 - (void)setLooping:(FLTLoopingMessage*)input error:(FlutterError**)error {
-  FLTVideoPlayer* player = _players[input.textureId];
-  [player setIsLooping:[input.isLooping boolValue]];
+  FLTVideoPlayer* player = self.playersByTextureId[input.textureId];
+  player.isLooping = input.isLooping.boolValue;
 }
 
 - (void)setVolume:(FLTVolumeMessage*)input error:(FlutterError**)error {
-  FLTVideoPlayer* player = _players[input.textureId];
-  [player setVolume:[input.volume doubleValue]];
+  FLTVideoPlayer* player = self.playersByTextureId[input.textureId];
+  [player setVolume:input.volume.doubleValue];
 }
 
 - (void)setPlaybackSpeed:(FLTPlaybackSpeedMessage*)input error:(FlutterError**)error {
-  FLTVideoPlayer* player = _players[input.textureId];
-  [player setPlaybackSpeed:[input.speed doubleValue]];
+  FLTVideoPlayer* player = self.playersByTextureId[input.textureId];
+  [player setPlaybackSpeed:input.speed.doubleValue];
 }
 
 - (void)play:(FLTTextureMessage*)input error:(FlutterError**)error {
-  FLTVideoPlayer* player = _players[input.textureId];
+  FLTVideoPlayer* player = self.playersByTextureId[input.textureId];
   [player play];
 }
 
 - (FLTPositionMessage*)position:(FLTTextureMessage*)input error:(FlutterError**)error {
-  FLTVideoPlayer* player = _players[input.textureId];
+  FLTVideoPlayer* player = self.playersByTextureId[input.textureId];
   FLTPositionMessage* result = [[FLTPositionMessage alloc] init];
   result.position = @([player position]);
   return result;
 }
 
 - (void)seekTo:(FLTPositionMessage*)input error:(FlutterError**)error {
-  FLTVideoPlayer* player = _players[input.textureId];
-  [player seekTo:[input.position intValue]];
-  [_registry textureFrameAvailable:input.textureId.intValue];
+  FLTVideoPlayer* player = self.playersByTextureId[input.textureId];
+  [player seekTo:input.position.intValue];
+  [self.registry textureFrameAvailable:input.textureId.intValue];
 }
 
 - (void)pause:(FLTTextureMessage*)input error:(FlutterError**)error {
-  FLTVideoPlayer* player = _players[input.textureId];
+  FLTVideoPlayer* player = self.playersByTextureId[input.textureId];
   [player pause];
 }
 
 - (void)setMixWithOthers:(FLTMixWithOthersMessage*)input
                    error:(FlutterError* _Nullable __autoreleasing*)error {
-  if ([input.mixWithOthers boolValue]) {
+  if (input.mixWithOthers.boolValue) {
     [[AVAudioSession sharedInstance] setCategory:AVAudioSessionCategoryPlayback
                                      withOptions:AVAudioSessionCategoryOptionMixWithOthers
                                            error:nil];

packages/video_player/video_player/pubspec.yaml

@@ -3,7 +3,7 @@ description: Flutter plugin for displaying inline video with other Flutter
   widgets on Android, iOS, and web.
 repository: https://github.com/flutter/plugins/tree/master/packages/video_player/video_player
 issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22
-version: 2.2.13
+version: 2.2.14
 
 environment:
   sdk: ">=2.14.0 <3.0.0"