diff --git a/packages/video_player/video_player_avfoundation/CHANGELOG.md b/packages/video_player/video_player_avfoundation/CHANGELOG.md index e2afaf91d44..982b1ae65fa 100644 --- a/packages/video_player/video_player_avfoundation/CHANGELOG.md +++ b/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. diff --git a/packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPTextureBasedVideoPlayer.m b/packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPTextureBasedVideoPlayer.m index 425fd6f7af8..52e90a4fb42 100644 --- a/packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPTextureBasedVideoPlayer.m +++ b/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]; diff --git a/packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m b/packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m index 4db671b8ab9..4667441cb97 100644 --- a/packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m +++ b/packages/video_player/video_player_avfoundation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m @@ -16,7 +16,64 @@ 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 *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 *observations, + NSObject *target) { + for (NSString *key in observations) { + [target removeObserver:observer forKeyPath:key]; + } +} + +/// Returns a mapping of KVO keys to NSValue-wrapped observer context pointers for observations that +/// should be set for AVPlayer instances. +static NSDictionary *FVPGetPlayerObservations(void) { + return @{ + @"rate" : [NSValue valueWithPointer:rateContext], + }; +} + +/// Returns a mapping of KVO keys to NSValue-wrapped observer context pointers for observations that +/// should be set for AVPlayerItem instances. +static NSDictionary *FVPGetPlayerItemObservations(void) { + return @{ + @"loadedTimeRanges" : [NSValue valueWithPointer:timeRangeContext], + @"status" : [NSValue valueWithPointer:statusContext], + @"presentationSize" : [NSValue valueWithPointer:presentationSizeContext], + @"duration" : [NSValue valueWithPointer:durationContext], + @"playbackLikelyToKeepUp" : [NSValue valueWithPointer:playbackLikelyToKeepUpContext], + }; +} + +@implementation FVPVideoPlayer { + // Whether or not player and player item listeners have ever been registered. + BOOL _listenersRegistered; +} - (instancetype)initWithURL:(NSURL *)url httpHeaders:(nonnull NSDictionary *)headers @@ -82,25 +139,33 @@ - (instancetype)initWithPlayerItem:(AVPlayerItem *)item }; _videoOutput = [avFactory videoOutputWithPixelBufferAttributes:pixBuffAttributes]; - [self addObserversForItem:item player:_player]; - [asset loadValuesAsynchronouslyForKeys:@[ @"tracks" ] completionHandler:assetCompletionHandler]; return self; } - (void)dealloc { - if (!_disposed) { - [self removeKeyValueObservers]; + if (_listenersRegistered && !_disposed) { + // If dispose was never called for some reason, remove observers to prevent crashes. + FVPRemoveKeyValueObservers(self, FVPGetPlayerItemObservations(), _player.currentItem); + FVPRemoveKeyValueObservers(self, FVPGetPlayerObservations(), _player); } } - (void)dispose { + // In some hot restart scenarios, dispose can be called twice, so no-op after the first time. + if (_disposed) { + return; + } _disposed = YES; - [self removeKeyValueObservers]; + + if (_listenersRegistered) { + [[NSNotificationCenter defaultCenter] removeObserver:self]; + FVPRemoveKeyValueObservers(self, FVPGetPlayerItemObservations(), self.player.currentItem); + FVPRemoveKeyValueObservers(self, FVPGetPlayerObservations(), self.player); + } [self.player replaceCurrentItemWithPlayerItem:nil]; - [[NSNotificationCenter defaultCenter] removeObserver:self]; if (_onDisposed) { _onDisposed(); @@ -108,40 +173,23 @@ - (void)dispose { [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)setEventListener:(NSObject *)eventListener { + _eventListener = eventListener; + // The first time an event listener is set, set up video event listeners to relay status changes + // changes to the event listener. + if (eventListener && !_listenersRegistered) { + AVPlayerItem *item = self.player.currentItem; + // If the item is already ready to play, ensure that the intialized event is sent first. + [self reportStatusForPlayerItem:item]; + // Set up all necessary observers to report video events. + FVPRegisterKeyValueObservers(self, FVPGetPlayerItemObservations(), item); + FVPRegisterKeyValueObservers(self, FVPGetPlayerObservations(), _player); + [[NSNotificationCenter defaultCenter] addObserver:self + selector:@selector(itemDidPlayToEndTime:) + name:AVPlayerItemDidPlayToEndTimeNotification + object:item]; + _listenersRegistered = YES; + } } - (void)itemDidPlayToEndTime:(NSNotification *)notification { @@ -228,17 +276,7 @@ - (void)observeValueForKeyPath:(NSString *)path [self.eventListener videoPlayerDidUpdateBufferRegions:values]; } else if (context == statusContext) { AVPlayerItem *item = (AVPlayerItem *)object; - switch (item.status) { - case AVPlayerItemStatusFailed: - [self sendFailedToLoadVideoEvent]; - break; - case AVPlayerItemStatusUnknown: - break; - case AVPlayerItemStatusReadyToPlay: - [item addOutput:_videoOutput]; - [self reportInitializedIfReadyToPlay]; - break; - } + [self reportStatusForPlayerItem:item]; } else if (context == presentationSizeContext || context == durationContext) { AVPlayerItem *item = (AVPlayerItem *)object; if (item.status == AVPlayerItemStatusReadyToPlay) { @@ -262,6 +300,20 @@ - (void)observeValueForKeyPath:(NSString *)path } } +- (void)reportStatusForPlayerItem:(AVPlayerItem *)item { + switch (item.status) { + case AVPlayerItemStatusFailed: + [self sendFailedToLoadVideoEvent]; + break; + case AVPlayerItemStatusUnknown: + break; + case AVPlayerItemStatusReadyToPlay: + [item addOutput:_videoOutput]; + [self reportInitializedIfReadyToPlay]; + break; + } +} + - (void)updatePlayingState { if (!_isInitialized) { return; @@ -436,17 +488,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 diff --git a/packages/video_player/video_player_avfoundation/pubspec.yaml b/packages/video_player/video_player_avfoundation/pubspec.yaml index c2e2ece35e4..a9d17a56b67 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 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