fix(darwin): Start observing AVPlayerItem.status before being assigned to AVPlayer#1549
Conversation
| onPreparedSubscription.cancel(); | ||
| try { | ||
| await fun(); | ||
| await preparedCompleter.future.timeout(const Duration(seconds: 30)); |
There was a problem hiding this comment.
I think this is not the right way here: we should avoid catching any error and instead try to not emit the prepared more than once or reinitialize the onPreparedCompleter.
|
Hi @blaugold thank you very much for contributing! We love that :) |
|
@blaugold little reminder, if you still working on this :) |
|
@Gustl22 Thanks for reviewing this PR and the ping. I'm still planning on addressing the comments, just haven't had the time yet :) |
# Conflicts: # packages/audioplayers_darwin/darwin/Classes/SwiftAudioplayersDarwinPlugin.swift # packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift
…audioplayers into fix/player-item-status
Gustl22
left a comment
Gustl22
left a comment
There was a problem hiding this comment.
I added another Pull request to better distinguish the refactor from the actual logical changes, for a better review. Hope that is ok for you.
| setUpSoundCompletedObserver(player, playerItem) | ||
| setUpPlayerItemStatusObservation(playerItem, completer, completerError) | ||
| let player = updatePlayer(playerItem) ?? createPlayer(playerItem) | ||
| setUpPositionObserver(player) |
There was a problem hiding this comment.
Setting up position observer for both creating and updating the player means that previously the position was not updated, when setting a new url? Is that right? So this is a fix then maybe? Or if not, why did it work in the first place?
There was a problem hiding this comment.
Now I get it: the position update is bound to the player instance, where as the completedObserver is bound to the playerItem instance.
I would propose the following:
- We move the
createPlayerpart to the init method or/and just useplayer = AVPlayer.init(), as this is only called once, during its live time. - We only use
updatePlayerinsetSourceUrl, but can be sure, that player is never nil. - Split observers in a
positionObserver(also initialized at the beginning) and acompletedObserver, reset every time onreset. ThepositionObserveris reset ondispose. - Replacing the
positonObserveron every source change doesn't really make sense, as the position updates keep on immediately, except url is nil. We should either remove them on every stop or leave them always running, I guess.
| } | ||
| } | ||
|
|
||
| playerItemStatusObservation?.invalidate() |
There was a problem hiding this comment.
Just as note for myself: Invalidate is now called in reset() for both creating and updating the player
# Description Pick refactor code of #1549 --------- Co-authored-by: Gabriel Terwesten <gabriel@terwesten.net>
# Conflicts: # packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift
|
@blaugold small reminder, if you still plan to work on this. Thank you ;D |
# Conflicts: # packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift
Gustl22
changed the title
AVPlayerItem.status before setting it on AVPlayerAVPlayerItem.status before being assigned to AVPlayer
|
@blaugold do you agree with the changes? Feel free to comment the concerns. I'll merge in the next few days :) |
…ned to `AVPlayer` (#1549) # Description * KV-Observer is now registered just after creating the `AVPlayerItem`. * Make player object non-nullable --------- Co-authored-by: Gustl22 <git@reb0.org>
3 participants
Description
Beginning with
4.1.0we are seeing a lot ofTimeoutException after 0:00:30.000000: Future not completedexceptions on iOS.I think I have tracked it down to when the KV-Observer for
AVPlayerItem.statusis registered. Before this change this was done after setting theAVPlayerItemon theAVPlayer. The problem with this is thatAVPlayerItemstarts loading immediately after setting it on theAVPlayerand can finish loading before the KV-Observer is even registered.Now the KV-Observer is registered just after creating the
AVPlayerItem.The fix could have been done with less changes, but I though
WrappedMediaPlayercould be clean up a bit.Checklist
fix:,feat:,docs:,chore:etc).///, where necessary.Breaking Change