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

Shuffle completely breaks the playlist's indexies #3

Open
Zensonaton opened this issue Jan 19, 2024 · 13 comments · May be fixed by #13
Open

Shuffle completely breaks the playlist's indexies #3

Zensonaton opened this issue Jan 19, 2024 · 13 comments · May be fixed by #13
Assignees
Labels
bug Something isn't working good first issue Good for newcomers
Milestone

Comments

@Zensonaton
Copy link

Hello! First of all, I wanted to say thank you for your package. Compared to just_audio_windows, your solution is much more stable.

Unfortunately, the shuffle feature is broken here: Calling setShuffleModeEnabled(true) results in a weird indexes being returned from the player class:

print(queue.sequence[player.currentIndex]) # Prints current track.
await player.setShuffleModeEnabled(true);

print(queue.sequence[player.currentIndex]) # Outputs the wrong track although the playlist haven't been changed.

Not only currentIndex is broken, but nextIndex (and probably previousIndex as well). Because of that issue, my UI displays the wrong track after user tries to enable the shuffle.

Second, just_audio's setShuffleModeEnabled can be called before calling player.setAudioSource, but media_kit doesn't allow that. Because of that, the playlist doesn't get shuffled.

Comparing just_audio_media_kit with native (?) Android version of just_audio, there aren't any issues with the shuffle: I can call the setShuffleModeEnabled before setting the playlist, and indecies are returning correctly.

@Pato05
Copy link
Owner

Pato05 commented Feb 13, 2024

Hi! As specified in the 'Caveats' section in the Readme, shuffle indices are currently not supported, and I couldn't work on supporting them for a lack of time, as media_kit itself doesn't support them either as far as I'm aware, so I'd need to manually change the queue (it only supports specifying whether the player has to shuffle or not).

As a temporary solution, you could manually shuffle the queue in your application and set that as the player's queue.
BTW, sorry for the late reply, I haven't checked GitHub for a while as I've been busy.

@Zensonaton
Copy link
Author

Thank you for your reply.

As specified in the 'Caveats' section in the Readme

Hm... I'm sorry, but I can't find that section in the README.md file. Are you certain that you have pushed the changes to the Github? 👀

shuffle indices are currently not supported, [...] as media_kit itself doesn't support them either as far as I'm aware

I've been using media_kit instead of just_audio in my project before. And yes, media_kit's shuffle implementation is awful, I can agree with that.
Due to awful shuffle/queue implementation on media_kit's side, I've had to rely on Spotube's solution: this implementation stores player's queue in a separate variable, and then feeds the player with currently playing media, one by one, instead of giving the player the complete playlist.

sorry for the late reply

Not a problem, better to be late than never :D

If you aren't planning to continue working on that package, then I may try something myself; can't promise anything tho :D

@Pato05
Copy link
Owner

Pato05 commented Feb 13, 2024

Hm... I'm sorry, but I can't find that section in the README.md file. Are you certain that you have pushed the changes to the Github? 👀

You're right.. I forgot to publish it. 💀

Spotube's solution: this implementation stores player's queue in a separate variable, and then feeds the player with currently playing media, one by one, instead of giving the player the complete playlist.

Yeah, I wanted to implement something like that in the plugin, so that shuffleIndices would be supported, just like natively on Android.

If you aren't planning to continue working on that package, then I may try something myself; can't promise anything tho :D

I am trying to work on it (also because I use it myself in my app), albeit slowly because of the lack of time. Though you're welcome to make your forks and eventually help me with development by submitting a PR.

@Chaphasilor
Copy link
Contributor

Hi, we're also trying to use this package for our media player (Finamp), but it seems like the queue isn't properly working for some reason. When one song is finished, the player stops playback (Stopping audio service) and the current MediaItem becomes null. Skipping to the next track works just fine, so I'm not sure where the problem is.
We're using a ConcatenatingAudioSource to play MP3s via HTTP, but the issue also appears when playing local files.

It seems like that bug is related to the discussion here, but I can also open a separate issue if you prefer :)

@Pato05
Copy link
Owner

Pato05 commented Mar 11, 2024

Hi, we're also trying to use this package for our media player (Finamp), but it seems like the queue isn't properly working for some reason. When one song is finished, the player stops playback (Stopping audio service) and the current MediaItem becomes null. Skipping to the next track works just fine, so I'm not sure where the problem is. We're using a ConcatenatingAudioSource to play MP3s via HTTP, but the issue also appears when playing local files.

It seems like that bug is related to the discussion here, but I can also open a separate issue if you prefer :)

Hi! The bug is not really related to this discussion, as this discussion is about the shuffle indexes not working (which is also stated in the README.md file). What you're talking about instead, is that the queue is completely broken, so yes, a new issue would be better. I'll start looking at the issue in the meantime, and also at the pull request you've opened.

@Pato05 Pato05 added the bug Something isn't working label Mar 13, 2024
@Pato05 Pato05 added this to the 2.1.0 milestone Apr 8, 2024
@Pato05 Pato05 self-assigned this Apr 8, 2024
@Pato05 Pato05 added the good first issue Good for newcomers label Apr 8, 2024
@Chaphasilor
Copy link
Contributor

Coming back to this now that my other issues have been resolved, I'd prefer a solution that fully updates media-kit's playlist instead of only feeding the next track.
I believe that makes more sense with regard to features like precaching (#11), and is also easier to implement (less edge-cases to handle; skipping back, forth, or jumping to a specific index works just like before). That is, unless there are major issues with replacing media-kit's playlist (we basically need to remove everything before and after the current track, and then add the new items before and after the current track).

I'm not sure if I'll have the time to play around with this myself, but I'd be happy to discuss this with you further!

@Zensonaton
Copy link
Author

Hello! I've made my own version of just_audio_media_kit that implements the solution I've been discussing before.

Not sure if I should make a PR for this, but here we are: Zensonaton/just_audio_media_kit. Basic features like seek (previous/next) are working as intended, while queue modification methods have not been tested.
In case if you want to check my repo without merging:

just_audio_media_kit:
  git:
    url: https://github.com/Zensonaton/just_audio_media_kit.git
    ref: main

@Chaphasilor
Copy link
Contributor

Chaphasilor commented Apr 18, 2024

@Zensonaton I just tried your fork, but it doesn't seem to be working for some reason. Toggling shuffle mode doesn't change the order of the tracks, neither in the effectiveSequence nor in what is actually being played.
I do get an error in the logs though:

UnimplementedError: setShuffleOrder() has not been implemented

which seems to originate from here:
https://github.com/ryanheise/just_audio/blob/6952be55c6ddfb01f2567c0737ef098d1bd9fd45/just_audio_platform_interface/lib/just_audio_platform_interface.dart#L124-L128

Is there anything I need to change about my code to make use of your implementation?

Edit: I also noticed that I don't get any changed order when using the published version of just_audio_media_kit. In your original comment you mentioned that the order changes for you, is that still the case?

@Zensonaton
Copy link
Author

@Chaphasilor, thank you for trying my fork.

I just tried your fork, but it doesn't seem to be working for some reason.

Ouch. This is why I wasn't comfortable in creating a PR :D
Interestingly, my own version of just_audio_media_kit works perfectly in my app.

Toggling shuffle mode doesn't change the order of the tracks, neither in the effectiveSequence nor in what is actually being played.

Hmmm, I thought that this is how things were supposed to be working. just_audio returns shuffleOrder only once, in the load method, this is why indecies aren't changing upon calling setShuffleMode multiple times.

setShuffleOrder() has not been implemented

Yeah, both Pato05's and mine implementations of just_audio_media_kit doesn't have a setShuffleOrder method. I've updated my fork to implement setShuffleOrder method. However, be warned! Due to lack of time on my side, I'm unable to properly test that change, so feel free to leave the feedback.

I also noticed that I don't get any changed order when using the published version of just_audio_media_kit. In your original comment you mentioned that the order changes for you, is that still the case?

Yeah- and no. In the original just_audio_media_kit library, shuffleOrder was completely ignored, as also stated in the README.md. My PR changes that behavior by creating its own queue, that plays media from given just_audio's queue one-by-one, while also respecting the shuffleOrder.

@Pato05
Copy link
Owner

Pato05 commented Apr 18, 2024

It's amazing @Zensonaton you've created a fork covering this! I have seen your commits, and really good job. Though your implementation as of right now would be incompatible with the prefetching I've introduced some commits ago. Maybe we could keep a cache of all the sources and then feed something like 4 at a time to the player, and whenever the shuffle order changes, we could just re-iterate over those few elements and add them to the underlying player.

Something like this would allow the player to pre-fetch the next track, and there wouldn't be any issues regarding the player automatically going to the next track (instead of having it be fed).

If you want, you could make a PR already, and mark it as a draft, so that I can also try suggesting changes. I just have to ask one thing before doing this: please merge the changes, since it's quite behind some commits.

@Pato05
Copy link
Owner

Pato05 commented Apr 18, 2024

Also @Chaphasilor if you need shuffling in your player, instead of using just_audio's methods, you can just shuffle the queue, and use that as new queue (by calling AudioPlayer.setAudioSource(...)). That's what I'm currently doing in my app.

If there are a lot of items in the queue, you can take a look at the compute function as to avoid freezing the main thread.

@Zensonaton Zensonaton linked a pull request Apr 19, 2024 that will close this issue
@Zensonaton
Copy link
Author

Take a look at the compute ...

Wait, I thought that the load method sends the queue to the underlying player in a separate thread. Thank you for telling me that, I'll definitely try that in my app 👀

@Chaphasilor
Copy link
Contributor

@Pato05 that's not really an option sadly. Not only would it mean re-implementing a lot of functionality (like our custom ShuffleOrder), but it would probably also break our playback history, and playback would be restarted when toggling shuffle.

Not having shuffle for now is better imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants