[video_player] Improve KVO handling on iOS

packages/video_player/video_player_avfoundation/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 2.8.3
+
+* Removes calls to self from init and dealloc, for maintainability.
+
 ## 2.8.2
 
 * Restructure internals of Dart notification of video player events.

packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPTextureBasedVideoPlayer.m

@@ -119,14 +119,6 @@ - (void)seekTo:(NSInteger)position completion:(void (^)(FlutterError *_Nullable)
 }
 
 - (void)dispose {
-  // This check prevents the crash caused by removing the KVO observers twice.
-  // When performing a Hot Restart, the leftover players are disposed once directly
-  // by [FVPVideoPlayerPlugin initialize:] method and then disposed again by
-  // [FVPVideoPlayer onTextureUnregistered:] call leading to possible over-release.
-  if (self.disposed) {
-    return;
-  }
-
   [super dispose];
 
   [self.playerLayer removeFromSuperlayer];

packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m

@@ -16,7 +16,49 @@
 static void *playbackLikelyToKeepUpContext = &playbackLikelyToKeepUpContext;
 static void *rateContext = &rateContext;
 
-@implementation FVPVideoPlayer
+/// Registers KVO observers on 'object' for each entry in 'observations', which must be a
+/// dictionary mapping KVO keys to NSValue-wrapped context pointers.
+///
+/// This does not call any methods on 'observer', so is safe to call from 'observer's init.
+static void FVPRegisterKeyValueObservers(NSObject *observer,
+                                         NSDictionary<NSString *, NSValue *> *observations,
+                                         NSObject *target) {
+  // It is important not to use NSKeyValueObservingOptionInitial here, because that will cause
+  // synchronous calls to 'observer', violating the requirement that this method does not call its
+  // methods. If there are use cases for specific pieces of initial state, those should be handled
+  // explicitly by the caller, rather than by adding initial-state KVO notifications here.
+  for (NSString *key in observations) {
+    [target addObserver:observer
+             forKeyPath:key
+                options:NSKeyValueObservingOptionNew
+                context:observations[key].pointerValue];
+  }
+}
+
+/// Registers KVO observers on 'object' for each entry in 'observations', which must be a
+/// dictionary mapping KVO keys to NSValue-wrapped context pointers.
+///
+/// This should only be called to balance calls to FVPRegisterKeyValueObservers, as it is an
+/// error to try to remove observers that are not currently set.
+///
+/// This does not call any methods on 'observer', so is safe to call from 'observer's dealloc.
+static void FVPRemoveKeyValueObservers(NSObject *observer,
+                                       NSDictionary<NSString *, NSValue *> *observations,
+                                       NSObject *target) {
+  for (NSString *key in observations) {
+    [target removeObserver:observer forKeyPath:key];
+  }
+}
+
+@implementation FVPVideoPlayer {
+  // A mapping of KVO keys to NSValue-wrapped observer context pointers for observations that should
+  // be set for the AVPlayer instance.
+  NSDictionary<NSString *, NSValue *> *_playerObservations;
+
+  // A mapping of KVO keys to NSValue-wrapped observer context pointers for observations that
+  // should be set for the AVPlayer instance's current AVPlayerItem.
+  NSDictionary<NSString *, NSValue *> *_playerItemObservations;
+}
 
 - (instancetype)initWithURL:(NSURL *)url
                 httpHeaders:(nonnull NSDictionary<NSString *, NSString *> *)headers
@@ -82,68 +124,55 @@ - (instancetype)initWithPlayerItem:(AVPlayerItem *)item
   };
   _videoOutput = [avFactory videoOutputWithPixelBufferAttributes:pixBuffAttributes];
 
-  [self addObserversForItem:item player:_player];
+  // Set up all necessary observers to report video events.
+  _playerItemObservations = @{
+    @"loadedTimeRanges" : [NSValue valueWithPointer:timeRangeContext],
+    @"status" : [NSValue valueWithPointer:statusContext],
+    @"presentationSize" : [NSValue valueWithPointer:presentationSizeContext],
+    @"duration" : [NSValue valueWithPointer:durationContext],
+    @"playbackLikelyToKeepUp" : [NSValue valueWithPointer:playbackLikelyToKeepUpContext],
+  };
+  _playerObservations = @{
+    @"rate" : [NSValue valueWithPointer:rateContext],
+  };
+  FVPRegisterKeyValueObservers(self, _playerItemObservations, item);
+  FVPRegisterKeyValueObservers(self, _playerObservations, _player);
+  [[NSNotificationCenter defaultCenter] addObserver:self
+                                           selector:@selector(itemDidPlayToEndTime:)
+                                               name:AVPlayerItemDidPlayToEndTimeNotification
+                                             object:item];
 
   [asset loadValuesAsynchronouslyForKeys:@[ @"tracks" ] completionHandler:assetCompletionHandler];
 
   return self;
 }
 
 - (void)dealloc {
-  if (!_disposed) {
-    [self removeKeyValueObservers];
-  }
+  // In case dispose was never called for some reason, remove any remaining observers to prevent
+  // crashes.
+  FVPRemoveKeyValueObservers(self, _playerItemObservations, _player.currentItem);
+  FVPRemoveKeyValueObservers(self, _playerObservations, _player);
 }
 
 - (void)dispose {
   _disposed = YES;
-  [self removeKeyValueObservers];
 
-  [self.player replaceCurrentItemWithPlayerItem:nil];
   [[NSNotificationCenter defaultCenter] removeObserver:self];
+  FVPRemoveKeyValueObservers(self, _playerItemObservations, self.player.currentItem);
+  FVPRemoveKeyValueObservers(self, _playerObservations, self.player);
+  // Clear the list of observations, so that dealloc (or potential duplicate dispose calls, in the
+  // case of hot restart) don't try to re-remove them, which would be an error.
+  _playerItemObservations = @{};
+  _playerObservations = @{};
+
+  [self.player replaceCurrentItemWithPlayerItem:nil];
 
   if (_onDisposed) {
     _onDisposed();
   }
   [self.eventListener videoPlayerWasDisposed];
 }
 
-- (void)addObserversForItem:(AVPlayerItem *)item player:(AVPlayer *)player {
-  [item addObserver:self
-         forKeyPath:@"loadedTimeRanges"
-            options:NSKeyValueObservingOptionInitial | NSKeyValueObservingOptionNew
-            context:timeRangeContext];
-  [item addObserver:self
-         forKeyPath:@"status"
-            options:NSKeyValueObservingOptionInitial | NSKeyValueObservingOptionNew
-            context:statusContext];
-  [item addObserver:self
-         forKeyPath:@"presentationSize"
-            options:NSKeyValueObservingOptionInitial | NSKeyValueObservingOptionNew
-            context:presentationSizeContext];
-  [item addObserver:self
-         forKeyPath:@"duration"
-            options:NSKeyValueObservingOptionInitial | NSKeyValueObservingOptionNew
-            context:durationContext];
-  [item addObserver:self
-         forKeyPath:@"playbackLikelyToKeepUp"
-            options:NSKeyValueObservingOptionInitial | NSKeyValueObservingOptionNew
-            context:playbackLikelyToKeepUpContext];
-
-  // Add observer to AVPlayer instead of AVPlayerItem since the AVPlayerItem does not have a "rate"
-  // property
-  [player addObserver:self
-           forKeyPath:@"rate"
-              options:NSKeyValueObservingOptionInitial | NSKeyValueObservingOptionNew
-              context:rateContext];
-
-  // Add an observer that will respond to itemDidPlayToEndTime
-  [[NSNotificationCenter defaultCenter] addObserver:self
-                                           selector:@selector(itemDidPlayToEndTime:)
-                                               name:AVPlayerItemDidPlayToEndTimeNotification
-                                             object:item];
-}
-
 - (void)itemDidPlayToEndTime:(NSNotification *)notification {
   if (_isLooping) {
     AVPlayerItem *p = [notification object];
@@ -436,17 +465,4 @@ - (int64_t)duration {
   return FVPCMTimeToMillis([[[_player currentItem] asset] duration]);
 }
 
-/// Removes all key-value observers set up for the player.
-///
-/// This is called from dealloc, so must not use any methods on self.
-- (void)removeKeyValueObservers {
-  AVPlayerItem *currentItem = _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"];
-  [_player removeObserver:self forKeyPath:@"rate"];
-}
-
 @end

packages/video_player/video_player_avfoundation/pubspec.yaml

@@ -2,7 +2,7 @@ name: video_player_avfoundation
 description: iOS and macOS 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.8.2
+version: 2.8.3
 
 environment:
   sdk: ^3.6.0