Skip to content
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.

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

Fix playback stopping after each track #4

Closed
wants to merge 2 commits into from

Conversation

Chaphasilor
Copy link
Contributor

So I've managed to fix the issue described in #3 by never returning ProcessingStateMessage.completed. That probably breaks stopping, but that's not as much of a problem in our case.
I haven't had time to look into in properly, but I'm guessing the check for when ProcessingStateMessage.completed should be returned instead of ProcessingStateMessage.ready isn't quiet correct, so I'm opening this draft PR to see if you might have an idea where to look for a proper fix?

@Pato05
Copy link
Owner

Pato05 commented Mar 11, 2024

This is a rather odd issue, as I'm using this library in my music app as well, and I have not encountered any issues with playing other items in queue, though I'm playing files from a remote URI instead of a local one, so that might be the reason why it works in my case and doesn't in yours...

@Pato05
Copy link
Owner

Pato05 commented Mar 11, 2024

I have tried to replicate the issue, but failed to do so, as my example is working (you can find it on https://github.com/Pato05/just_audio_media_kit_example).

Please try checking if your code might be doing something wrong, or, otherwise, please try to make a reproducible example so that I can check it, thus test the library against it to find a fix.

ProcessingStateMessage.completed, even if sent, shouldn't trigger anything in just_audio by itself, as the underlying player would still be allowed to continue playback in the next queue item (and it should).

Also, maybe try running your application in debug mode, and providing logs so that we can analyze those and check whether is there anything wrong.

@Chaphasilor
Copy link
Contributor Author

Chaphasilor commented Mar 12, 2024

I did some initial testing (Windows, with media_kit_libs_windows_audio):

  • (Continuous) Playback in the example project is working, using local files, URIs and embedded files
  • I noticed the example doesn't use pre-loading, but disabling that in our app didn't help

What is probably the culprit is this section:

_player.processingStateStream.listen((event) {
  if (event == ProcessingState.completed) {
    stop();
  }
});

(https://github.com/jmshrv/finamp/blob/ac09018ff7db3dabce38bf60e315df9fc67c83e4/lib/services/music_player_background_task.dart#L196-L200)

The question is: why is the behavior different from regular just_audio? We've been running that code for over a year on Android and iOS and never had such an issue with it. It seems like just_audio is not sending out that state when just_audio_media_kit is.

@Chaphasilor
Copy link
Contributor Author

Chaphasilor commented Mar 13, 2024

Expanding on this, I think there's a fundamental semantic difference between just_audio's ProcessingState.completed and media-kit's Player.stream.completed:

The just_audio documentation (https://pub.dev/documentation/just_audio/latest/just_audio/ProcessingState.html) mentions:

completed → const ProcessingState
The player has reached the end of the audio.

"audio" in this case seems to refer to an AudioSource, at least that is what all the other ProcessingStates are based upon. And since ConcatenatingAudioSource is an AudioSource (see "Implementers at https://pub.dev/documentation/just_audio/latest/just_audio/AudioSource-class.html), this should mean that ProcessingState.completed should only be dispatched when the end of the ConcatenatingAudioSource has been reached (and loop mode is off).

media-kit on the other hand offers the following documentation (based on doc comments in player_stream.dart):

/// Currently opened [Media]s.
final Stream<Playlist> playlist;

...

/// Whether end of currently playing [Media] has been reached.
final Stream<bool> completed;

Since the playlist (probably the equivalent of a ConcatenatingAudioSource) is described as a set of multiple Medias, the Player.stream.completed stream seems to fire an event every time a single track has finished playing. At least that would explain the behavior we're seeing.

A better approach might be to use the Player.stream.playlist stream, comparing the Playlists.index to the length of Playlist.medias, and then checking for the Player.stream.completed event only if the last index is playing, all while also respecting the PlaylistMode.
I've implemented this with the latest commit, and it seems to be working.

@Pato05
Copy link
Owner

Pato05 commented Mar 13, 2024

Very interesting.. I wanna thank you for the time you invested in investigating this further, and for your findings. I'll take a better look at this later.

@Pato05 Pato05 marked this pull request as ready for review March 13, 2024 15:37
@Pato05
Copy link
Owner

Pato05 commented Mar 13, 2024

Could you also squash your commits into one, and change the commit message to something more specific like "set ProcessingStateMessage.completed only on Playlist end"? Otherwise I could make a Co-Authored commit and close this PR.

@Chaphasilor
Copy link
Contributor Author

I created the PR with github.dev, and there's not full git integration. I could clone it and do the squash, or you can just use the "Squash and merge" option to merge the PR?
Otherwise, feel free to co-author :)

Pato05 added a commit that referenced this pull request Mar 13, 2024
avoid calling `_updatePlaybackEvent()` whenever not needed

Co-Authored-By: Chaphasilor <ppp.u@web.de>
@Pato05
Copy link
Owner

Pato05 commented Mar 13, 2024

Merged manually.

@Pato05 Pato05 closed this Mar 13, 2024
@Pato05 Pato05 added the enhancement New feature or request label Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants