I wrote a UIView subclass "VideoPlayerView" to encapsulate AVFoundation video playback. I believed that I had a bulletproof KVO pattern set up to handle observation of the AVPlayer, AVPlayerItems and AVURLAssets for the purpose of loading, playback and error handling.
Instead, I find crashes being reported that this pattern was specifically set up to guard against (rarely, but reported nonetheless).
a) An instance 0x170019730 of class AVPlayerItem was deallocated while key value observers were still registered with it.
b) [VideoPlayerView setPlayerItem:] Cannot remove observer VideoPlayerView for the key path "status" from AVPlayerItem because it is not registered as an observer.
c) [VideoPlayerView setAsset:] Cannot remove an observer VideoPlayerView 0x145e3bbd0 for the key path "playable" from AVURLAsset 0x170233780 because it is not registered as an observer.
I would like to learn why these errors are occurring, what it is that I have missed or misunderstood and how to make things more robust.
Specific details are simplified for the purpose of the explanation, but I believe all relevant information is here.
I have a class VideoPlayerView, it holds these properties amongst others:
@property (strong, nonatomic) AVPlayerItem *playerItem;
@property (strong, nonatomic) AVURLAsset *asset;
@property (strong, nonatomic, readonly) AVPlayerLayer *playerLayer;
Note that all references are strong - these objects cannot be deallocated until the VideoPlayerView (which is doing the observing) is itself deallocated. AVPlayerLayer maintains a strong reference to its AVPlayer property.
I implement custom getters as follows:
- (AVPlayer*)player
{
return [(AVPlayerLayer*)self.layer player];
}
- (AVPlayerLayer *)playerLayer
{
return (AVPlayerLayer *)self.layer;
}
I implement custom setters as follows:
- (void) setPlayer:(AVPlayer*)player
{
// Remove observation for any existing player
AVPlayer *oldPlayer = [self player];
[oldPlayer removeObserver:self forKeyPath:kStatus];
[oldPlayer removeObserver:self forKeyPath:kCurrentItem];
// Set strong player reference
[(AVPlayerLayer*)[self layer] setPlayer:player];
// Add observation for new player
[player addObserver:self forKeyPath:kStatus options:NSKeyValueObservingOptionNew context:kVideoPlayerViewKVOContext];
[player addObserver:self forKeyPath:kCurrentItem options:NSKeyValueObservingOptionNew context:kVideoPlayerViewKVOContext];
}
- (void) setAsset:(AVURLAsset *)asset
{
// Remove observation for any existing asset
[_asset removeObserver:self forKeyPath:kPlayable];
// Set strong asset reference
_asset = asset;
// Add observation for new asset
[_asset addObserver:self forKeyPath:kPlayable options:NSKeyValueObservingOptionNew context:kVideoPlayerViewKVOContext];
}
- (void) setPlayerItem:(AVPlayerItem *)playerItem
{
// Remove observation for any existing item
[_playerItem removeObserver:self forKeyPath:kStatus];
NSNotificationCenter *nc = [NSNotificationCenter defaultCenter];
[nc removeObserver:self name:AVPlayerItemDidPlayToEndTimeNotification object:_playerItem];
[nc removeObserver:self name:AVPlayerItemPlaybackStalledNotification object:_playerItem];
[nc removeObserver:self name:AVPlayerItemFailedToPlayToEndTimeNotification object:_playerItem];
// Set strong playerItem reference
_playerItem = playerItem;
// Add observation for new item
[_playerItem addObserver:self forKeyPath:kStatus options:NSKeyValueObservingOptionNew context:kVideoPlayerViewKVOContext];
if (_playerItem)
{
[nc addObserver:self selector:@selector(handlePlayerItemDidReachEndTimeNotification:) name:AVPlayerItemDidPlayToEndTimeNotification object:_playerItem];
[nc addObserver:self selector:@selector(handlePlayerItemFailureNotification:) name:AVPlayerItemPlaybackStalledNotification object:_playerItem];
[nc addObserver:self selector:@selector(handlePlayerItemFailureNotification:) name:AVPlayerItemFailedToPlayToEndTimeNotification object:_playerItem];
}
}
Outside of these custom setters, VideoPlayerView always uses "self.property =" or "[self setProperty:]" and never "_property =", so that the custom setter is always used.
Finally, VideoPlayerView implements a dealloc method as follows:
- (void) dealloc
{
[self releasePlayerAndAssets];
}
- (void) releasePlayerAndAssets
{
[self setAsset:nil];
[self setPlayerItem:nil];
[self setPlayer:nil];
}
Yes, I should just inline this pointless abstraction! Nevertheless, this means that on deallocation of the VideoPlayerView, any strong properties therein have their observation removed, and are then released to allow their deallocation.
So then, I believe this pattern should mitigate the crashes I am observing as follows:
a) An instance 0x170019730 of class AVPlayerItem was deallocated while key value observers were still registered with it.
VideoPlayerView is the only class of mine observing AVPlayerItem. VideoPlayerView maintains a strong reference to AVPlayerItem all the while that it is observing it. Therefore AVPlayerItem cannot be deallocated while VideoPlayerView is alive, and prior to its deallocation VideoPlayerView will stop observing the AVPlayerItem prior to AVPlayerItem's subsequent deallocation.
How is this going wrong?
b) [VideoPlayerView setPlayerItem:] Cannot remove observer VideoPlayerView for the key path "status" from AVPlayerItem because it is not registered as an observer.
c) [VideoPlayerView setAsset:] Cannot remove an observer VideoPlayerView 0x145e3bbd0 for the key path "playable" from AVURLAsset 0x170233780 because it is not registered as an observer.
My custom setters are trying to remove the observation of any previously set AVPlayerItem or AVURLAsset prior to replacing the property with the pointer to the new or incoming AVPlayerItem or AVURLAsset.
When my class is instantiated, _playerItem and _asset are nil. Therefore any previous AVPlayerItem or AVURLAsset must have been set through the custom setter and therefore have VideoPlayerView registered as an observer for those keypaths.
How are these properties being set without observation being set up?
Are these just horrible race conditions based on the order of method calls in the custom setters?
Is there something fundamental I am missing here?
I am considering using the objective-c runtime to create an associated object property BOOL isObserved on these objects just to be able to do a sanity check before trying to remove observer. I get the feeling even this won't be robust enough given the issues with the current methodology.
Any insight or help much appreciated. Thank you for reading.
After lengthy conversations with Apple engineers, the take away message seems to be that deregistering KVO observation in the dealloc method of the observing class is not a good pattern. Apple's KVO guide does recommend not using custom setters or getters in the init and dealloc method, however I was told that the documentation's language should have been much stronger on this point - it should never be done.
Essentially, it is never guaranteed to work due to the complexities of KVOs implementation. It MAY work in certain cases, but it is never guaranteed and displays a high degree of unpredictability - random crashes are almost to be expected unless the case is very simple.
Some choice excerpts from my correspondence with Apple regarding this pattern follow, paraphrased for SO:
The challenge here is the broad span of how people interact with KVO and how more complex usage patterns shift behaviors around. In the simple case of an NSObject subclass observing another object, there really isn’t much of an issue. It’s when the situation becomes more complex that things start to breakdown and get a lot uglier. When you spend a lot of time staring at the weird edges cases that break, you get a lot more paranoid in your approach.
KVO’s relative age and history on macOS is part of this as well. Compared to iOS, macOS apps generally have much simpler subclassing patterns - there is no ViewController class in the same way as iOS and they tend to rely heavily on the standard UI classes, so it’s not at all unusual for most of the classes in a macOS app to inherit directly from NSObject.
Basically, the problem here is that many simple case work just fine, and the complicated cases… can be really, really weird. These problems are not unknown, but the fact that lots of developers have had it “just work” in their app means they’re not necessarily that visible.
Here is an decent overview of that perspective: http://khanlou.com/2013/12/kvo-considered-harmful/
In summation:
Ideally KVO should be set and unset at well-defined logical points in the life of the involved classes, and not rely on dealloc wherever possible. Clearly there are some cases where this is not going to be possible - where observation must occur throughout the life of an object that could be deallocated at an undisclosed point (i.e. managed by iOS, such as a recycled collection view cell) - and in those cases I was recommended to use a separate wrapper class to handle the KVO.
Rather than write my own, I researched and decided to use Lily Ballard's excellent PMKVObserver wrapper class. It is extremely convenient, thread-safe and handles unregistration automatically on the death of either the observer or observing object.
https://github.com/postmates/PMKVObserver
At the time of writing, all these exceptions have disappeared in the build using PMKVObserver in place of this dealloc-unregistration pattern.