[ASVideoPlayerNode] Fixes and improvements #3135
Conversation
Source/ASVideoPlayerNode.mm
Outdated
|
|
||
| // Set asset based on interface state | ||
| if ((ASInterfaceStateIncludesPreload(self.interfaceState))) { | ||
| _videoNode.asset = asset; |
There was a problem hiding this comment.
Also, it maybe a good idea to avoid holding the instance lock while calling the video node. Doing so might potentially cause deadlocks if videoNode (or a subclass) for some reasons walks up the node hierarchy.
There was a problem hiding this comment.
Lastly, please consider adding thread assertions since these setters are probably main-thread only?
Source/ASVideoPlayerNode.mm
Outdated
| if ([_pendingAsset isKindOfClass:AVURLAsset.class]) { | ||
| return ((AVURLAsset *)_pendingAsset).URL; | ||
| } else { | ||
| return _videoNode.assetURL; |
There was a problem hiding this comment.
Same here, avoid holding instance lock while calling a subnode. Although in case of getters, the risk is very small.
Source/ASVideoPlayerNode.mm
Outdated
| return [_delegate videoPlayerNodeNeededDefaultControls:self]; | ||
| // If we enter preload state we apply the pending asset to load to the video node so it can start and fetch the asset | ||
| if (_pendingAsset != nil && _videoNode.asset != _pendingAsset) { | ||
| _videoNode.asset = _pendingAsset; |
There was a problem hiding this comment.
Same here, don't hold instance lock.
|
@nguyenhuy Comments addressed. Can you take another look over it. Thanks! |
Source/ASVideoPlayerNode.mm
Outdated
| { | ||
| if (!(self = [super init])) { | ||
| return nil; | ||
| __instanceLock__.lock(); |
There was a problem hiding this comment.
We probably don't need this?
There was a problem hiding this comment.
Thanks for catching that ...
This PR is building on top of the fabulous work of @nguyenhuy in #3093.
Some problems in ASVideoPlayerNode currently:
ASVideoPlayerNodeand the other one is within the backingASVideoNode. Ideally we only want to have only one final source of truth, therefore this PR will try to use theASVideoNodeas final truth of source._loadAssetWhenNodeBecomesVisibleisn't turned on in the empty initializer. This causes an assertion in [ASVideoPlayerNode] Crash initialising videoplayer node #3042 because it's not safe to set an asset on ASVideoNode off main.This PR deprecates _loadAssetWhenNodeBecomesVisible and fixes [ASVideoPlayerNode] Crash initialising videoplayer node #3042 by letting ASVideoPlayerNode handles its objects automatically.
@MarvinNazari Would be great if you can comment and help me to test this PR. Thank you!