fix(darwin): Start observing `AVPlayerItem.status` before being assigned to `AVPlayer` #1549

blaugold · 2023-06-17T10:39:00Z

Description

Beginning with 4.1.0 we are seeing a lot of TimeoutException after 0:00:30.000000: Future not completed exceptions on iOS.

I think I have tracked it down to when the KV-Observer for AVPlayerItem.status is registered. Before this change this was done after setting the AVPlayerItem on the AVPlayer. The problem with this is that AVPlayerItem starts loading immediately after setting it on the AVPlayer and 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 WrappedMediaPlayer could be clean up a bit.

Checklist

The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs:, chore: etc).
I have read the Contributor Guide and followed the process outlined for submitting PRs.
I have updated/added tests for ALL new/updated/fixed functionality.
I have updated/added relevant documentation and added dartdoc comments with ///, where necessary.
I have updated/added relevant examples in example.

Breaking Change

Yes, this is a breaking change.
No, this is not a breaking change.

…ayer`

Gustl22 · 2023-06-26T11:48:41Z

packages/audioplayers/lib/src/audioplayer.dart

-    onPreparedSubscription.cancel();
+    try {
+      await fun();
+      await preparedCompleter.future.timeout(const Duration(seconds: 30));


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.

Gustl22 · 2023-06-26T11:51:17Z

Hi @blaugold thank you very much for contributing! We love that :)
Would you mind to split the refactor part (in a new PR) from the actual fix, otherwise the changes are hard to track, and then maybe we could add a test to avoid reverting the fix? Thank you for your help!

Gustl22 · 2023-07-18T20:35:31Z

@blaugold little reminder, if you still working on this :)

blaugold · 2023-07-18T20:46:18Z

@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

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.

Gustl22 · 2023-07-28T22:26:49Z

packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift

            setUpPlayerItemStatusObservation(playerItem, completer, completerError)
+            let player = updatePlayer(playerItem) ?? createPlayer(playerItem)
+            setUpPositionObserver(player)


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?

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 createPlayer part to the init method or/and just use player = AVPlayer.init(), as this is only called once, during its live time.

We only use updatePlayer in setSourceUrl, but can be sure, that player is never nil.

Split observers in a positionObserver (also initialized at the beginning) and a completedObserver, reset every time on reset. The positionObserver is reset on dispose.

Replacing the positonObserver on 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.

packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift

Gustl22 · 2023-07-28T22:32:07Z

packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift

                self.reset()
                completerError?()
            default:
                break
            }
        }
-
-        playerItemStatusObservation?.invalidate()


Just as note for myself: Invalidate is now called in reset() for both creating and updating the player

packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift

# Description Pick refactor code of #1549 --------- Co-authored-by: Gabriel Terwesten <gabriel@terwesten.net>

# Conflicts: # packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift

Gustl22 · 2023-08-18T11:39:20Z

@blaugold small reminder, if you still plan to work on this. Thank you ;D

# Conflicts: # packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift

Gustl22 · 2023-09-25T07:49:06Z

@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> (cherry picked from commit 8c3a213)

…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>

blaugold added 3 commits June 17, 2023 12:32

fix: start observing AVPlayerItem.status before setting it on `AVPl…

83ff794

…ayer`

Remove debug print

257a6a7

Fix spelling

aa9449c

spydon approved these changes Jun 26, 2023

View reviewed changes

spydon requested a review from Gustl22 June 26, 2023 08:53

Gustl22 reviewed Jun 26, 2023

View reviewed changes

Gustl22 and others added 10 commits July 28, 2023 12:18

Merge branch 'main' into fix/player-item-status

d8ff1a7

chore: update darwin dependencies

711fe1f

refactor(darwin): cast maps

58b1868

refactor: format

47388e8

refactor: rearrange methods

b4eb212

refactor: releaseSync to release

f2ba251

reafactor(darwin): create player item as own method

b11351a

refactor(darwin): setUpPlayerItemStatusObservation as own method

20657c7

refactor(darwin): setUpPositionObserver as own method

f922ed5

refactor(darwin): setUpSoundCompletedObserver

fda9f4d

Gustl22 mentioned this pull request Jul 28, 2023

refactor(darwin): Rearrange code #1585

Merged

7 tasks

Gustl22 changed the base branch from main to gustl22/darwin-refactor July 28, 2023 22:05

Gustl22 added 2 commits July 29, 2023 00:09

Merge branch 'gustl22/darwin-refactor' into fix/player-item-status

51b49e7

# Conflicts: # packages/audioplayers_darwin/darwin/Classes/SwiftAudioplayersDarwinPlugin.swift # packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift

Merge branch 'fix/player-item-status' of https://github.com/blaugold/…

dabf06f

…audioplayers into fix/player-item-status

Gustl22 reviewed Jul 28, 2023

View reviewed changes

packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift Outdated Show resolved Hide resolved

Gustl22 deleted the branch bluefireteam:main August 1, 2023 10:35

Gustl22 added a commit that referenced this pull request Aug 1, 2023

refactor(darwin): Rearrange code (#1585)

13639d1

# Description Pick refactor code of #1549 --------- Co-authored-by: Gabriel Terwesten <gabriel@terwesten.net>

Gustl22 closed this Aug 1, 2023

Gustl22 reopened this Aug 1, 2023

Gustl22 changed the base branch from gustl22/darwin-refactor to main August 1, 2023 13:00

Merge remote-tracking branch 'upstream/main' into fix/player-item-status

ba4d121

# Conflicts: # packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift

Gustl22 added 3 commits September 9, 2023 03:03

Merge branch 'main' into fix/player-item-status

d4b2812

# Conflicts: # packages/audioplayers_darwin/darwin/Classes/WrappedMediaPlayer.swift

swift-format

3403df8

Merge remote-tracking branch 'upstream/main' into fix/player-item-status

c79df69

Gustl22 changed the title ~~fix: start observing AVPlayerItem.status before setting it on AVPlayer~~ fix(darwin): Start observing AVPlayerItem.status before being assigned to AVPlayer Sep 23, 2023

Gustl22 added 7 commits September 23, 2023 20:06

avoid catching unknown errors

69b347c

remove log

2634da6

darwin: simplify assignment

52de571

darwin: observer improvements

00d0285

darwin: Make player not nullable

67478d4

recreate ios folder

2c62ba7

recreate macos folder

9429b76

Gustl22 mentioned this pull request Sep 25, 2023

test: Recreate darwin example folders #1654

Merged

7 tasks

Merge branch 'main' into fix/player-item-status

98c04b6

Gustl22 merged commit 8c3a213 into bluefireteam:main Sep 26, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(darwin): Start observing `AVPlayerItem.status` before being assigned to `AVPlayer` #1549

fix(darwin): Start observing `AVPlayerItem.status` before being assigned to `AVPlayer` #1549

blaugold commented Jun 17, 2023 •

edited

Loading

Gustl22 Jun 26, 2023

Gustl22 commented Jun 26, 2023 •

edited

Loading

Gustl22 commented Jul 18, 2023

blaugold commented Jul 18, 2023

Gustl22 left a comment

Gustl22 Jul 28, 2023

Gustl22 Aug 27, 2023 •

edited

Loading

Gustl22 Jul 28, 2023

Gustl22 commented Aug 18, 2023

Gustl22 commented Sep 25, 2023

fix(darwin): Start observing AVPlayerItem.status before being assigned to AVPlayer #1549

fix(darwin): Start observing AVPlayerItem.status before being assigned to AVPlayer #1549

Conversation

blaugold commented Jun 17, 2023 • edited Loading

Description

Checklist

Breaking Change

Gustl22 Jun 26, 2023

Choose a reason for hiding this comment

Gustl22 commented Jun 26, 2023 • edited Loading

Gustl22 commented Jul 18, 2023

blaugold commented Jul 18, 2023

Gustl22 left a comment

Choose a reason for hiding this comment

Gustl22 Jul 28, 2023

Choose a reason for hiding this comment

Gustl22 Aug 27, 2023 • edited Loading

Choose a reason for hiding this comment

Gustl22 Jul 28, 2023

Choose a reason for hiding this comment

Gustl22 commented Aug 18, 2023

Gustl22 commented Sep 25, 2023

fix(darwin): Start observing `AVPlayerItem.status` before being assigned to `AVPlayer` #1549

fix(darwin): Start observing `AVPlayerItem.status` before being assigned to `AVPlayer` #1549

blaugold commented Jun 17, 2023 •

edited

Loading

Gustl22 commented Jun 26, 2023 •

edited

Loading

Gustl22 Aug 27, 2023 •

edited

Loading