Skip to content

[video_player] Improve KVO handling on iOS #9718

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
auto-submit merged 7 commits into from
Aug 7, 2025
Merged

[video_player] Improve KVO handling on iOS #9718

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f3d30e4
Move KVO to flexible static helpers
stuartmorgan-g Jul 31, 2025
f4e17b6
Remove KVO initial calls
stuartmorgan-g Jul 31, 2025
ffecb85
Version bump
stuartmorgan-g Jul 31, 2025
cef5691
Typo fix
stuartmorgan-g Aug 1, 2025
f9b266f
Use _disposed instead of modifying registration lists
stuartmorgan-g Aug 4, 2025
9ad7fe7
Move listener registration out of init
stuartmorgan-g Aug 5, 2025
588b33e
C warning fix
stuartmorgan-g Aug 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/video_player/video_player_avfoundation/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
8 changes: 0 additions & 8 deletions .../video_player_avfoundation/Sources/video_player_avfoundation/FVPTextureBasedVideoPlayer.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
128 changes: 72 additions & 56 deletions ...ation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 'obsever'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
Expand Down Expand Up @@ -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);
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the original implementation specified NSKeyValueObservingOptionInitial, but here I don't see how the initial values are going to be reported?

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PR description it says:

The handlers for most keys just collect information to send to the event listener, but there is no event listener during init (it's set just after), so those were expensive no-ops.
The handlers related to initialization require the item to be in the AVPlayerItemStatusReadyToPlay state, and items start in AVPlayerItemStatusUnknown, so the initial state call won't do anything useful.

So this means the item is always in .unknown state when the event listener is set? Is it possible to get an item that's already .failed?

Copy link
Collaborator Author

@stuartmorgan-g stuartmorgan-g Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not according to the docs:

When a player item is created, its status is AVPlayerItem.Status.unknown, meaning its media hasn’t been loaded and has not yet been enqueued for playback. Associating a player item with an AVPlayer immediately begins enqueuing the item’s media and preparing it for playback.

Copy link
Contributor

@LongCatIsLooong LongCatIsLooong Aug 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I feel that doesn't guarantee that AVPlayer.status can't change synchronously especially when the KVO registration happens after _player = [avFactory playerWithPlayerItem:item]; (which I assume is when the player item is associated with an AVPlayer). On the other hand I agree that it probably doesn't make too much sense for AVFoundation to change the status synchronously (maybe if the AVPlayer implementation has logic that does simple sanity checks as soon as a new item is assigned?). Should we add an assert before the registration to make sure the status is .unknown?

Copy link
Collaborator Author

@stuartmorgan-g stuartmorgan-g Aug 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

especially when the KVO registration happens after _player = [avFactory playerWithPlayerItem:item]; (which I assume is when the player item is associated with an AVPlayer)

That's a great point; I wasn't considering the order of operations there if the internals make synchronous status changes. Rather than relying on undocumented behavior and asserting, I've instead moved the KVO registration to setEventListener:. That makes things much easier to reason about, because we're no longer doing any of this from init, so we can call whatever we want, and unlike the assert means clients won't get incorrect runtime behavior if AVPlayer ever starts actually doing that.

Rather than add back NSKeyValueObservingOptionInitial (which would cause a bunch of initial state event updates in Dart that didn't used to exist, and in a random order), I've added an explicit call to check the state and, if it has already changed, send an initialization event (which is now safe because this isn't calling methods from init) before registering the listeners.

(The listener is set right after init, so in practice this will have almost no effect on when any of this happens, but I think this structure is much cleaner.)

[[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];
Expand Down Expand Up @@ -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
2 changes: 1 addition & 1 deletion packages/video_player/video_player_avfoundation/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down