-
Notifications
You must be signed in to change notification settings - Fork 12
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
New queue playback solution #13
base: main
Are you sure you want to change the base?
Conversation
If we were to go with an implementation that feeds the next X tracks, it would make sense to make that number dynamic depending on the cache size/duration. |
Thank you for this, I will take a more thorough look in the next days |
Hello! I'm kinda curious whether there is progress with this PR. No rush, just curious 👀 |
There hasn't been any, I'm currently very busy with school exams.. I'll try to check this if I have any free time. Anyways, my idea is that we could feed the next 2 or 3 tracks to media-kit so that we can prefetch them (maybe implement this under a method so that we can reuse the code?). This should happen every time the track is changed. |
Oh, I really hope that my comment didn't look like I was rushing you or anything, sorry 'bout that. I wish you luck with the exams. 🤞 Speaking of your solution about feeding player next N media, that can be implemented pretty much easily in my fork, so I might try that. The only thing that I need to keep in mind is the fact that MK's player should never be shuffled, otherwise things might get really badly. |
Oh, don't worry! I didn't feel rushed at all, probably my comment sounded way too harsh, sorry. |
No idea why I marked this PR as draft, lol :D |
So you're not planning to implenent the pre-fetching for now? :) |
Tried to do so- I'm not 100% sure how to do that without any disgusting hacks. |
rename some variables, make some code reusable (so that it looks a bit cleaner) fix the indexes (which just_audio already corrects for them to be according to the shuffled indices) keep [_shuffleOrder] updated in removeRange and move requests
@Zensonaton I want to congratulate you for this implementation (which to my surprise worked almost perfectly, which is unusual since this is kind of hacky). I fixed some things (as you can see by my commits), and I also merged it to the latest changes. I'd just wait to implement prefetching (by including the next X items in the playlist, as I said), then we can merge this PR. |
Glad to hear this! Happy to see the changes made.
There's a bit of a problem with that approach; my code subscribes to MK's While it's possible to subscribe to the media index and other events, however, that'll require more wacky-hacky code :D |
Yeah.. I overlooked the issue indeed: added to what you already said (which can easily be fixed by listening to whenever the current entry changes, thus regenerating the playlist), I might test it anyways, and check if it seems to work. If it does, great, we can take this approach, otherwise I'll make it so that enabling |
…ting, etc. Fix wrong indecies being used with enabled shuffle. Fix issues with loop mode. Fix wrong media being played on loop. Use `await _player.method` instead of `_player.method.then(...)`. More loggers.
It seems like I've successfully implemented the prefetching. As it turned out, it's been more easier than I've thought it would be :D just_audio_media_kit:
git:
url: https://github.com/Zensonaton/just_audio_media_kit.git
ref: "529af19" and set JustAudioMediaKit.title = "My Spotify competitor app";
JustAudioMediaKit.prefetchPlaylist = true;
JustAudioMediaKit.ensureInitialized(); Not only that, new commit introduced a lot of QoL-changes to the Also... It seems like @Pato05's "fix a few things" commit broke my entire shuffle implementation. I've had a lot of time trying to understand why my music player wasn't working properly, lol :D |
@Zensonaton great to hear! I'll try to take a look at this tomorrow by plugging it into Finamp. Shuffle was broken on desktop for us anyway, but I'll see what works and what doesn't! Please remind me if I forget! |
@Chaphasilor , hope that'll work properly for you. I'm curious, why was the shuffle broken in your app? |
Could you elaborate on that? That commit pretty much had the effect of setting Also, about Would you mind telling me how you tested the package? I have personally used the example app of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About this commit, as I've stated in my previous comment:
--prefetch-playlist
expects a static playlist, and having a dynamic one with next and previous X elements will not satisfy this requirement
Did you test this implementation thoroughly? Like, did you see if any data gets discarded even if not needed?
registerWith(); | ||
MediaKit.ensureInitialized(libmpv: libmpv); | ||
} | ||
(UniversalPlatform.isMacOS && macOS))) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be done. It should be the user's choice whether to enable just_audio_media_kit
to be used instead of the default plugin on macOS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused about this one... I've cleaned my code, so brackets won't confuse you:
if (!(
(UniversalPlatform.isLinux && linux) ||
(UniversalPlatform.isWindows && windows) ||
(UniversalPlatform.isAndroid && android) ||
(UniversalPlatform.isIOS && iOS) ||
(UniversalPlatform.isMacOS && macOS)
)) return;
I haven't removed the isMacOS
check, it is still present in my code. Or am I missing something here?
Basically, my code does the exact same thing as your did, but instead of this:
if (condition) {
do_something();
}
I've changed it to this:
if (!condition) return;
do_something();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I'm sorry, I didn't read it properly.
Though readability-wise, I liked it more my way, it's more straightforward since it's just one "if" statement, and a pretty long one, too.
Thank you @Pato05 for checking my commits.
My music app completely broke after commit of yours: basically, I've had an issue, where user plays song A, but my app starts playing completely random song of a playlist. Basically, my app calls this method to start playing music: await player.setAudioSource(
ConcatenatingAudioSource(...),
initialIndex: selectedSongIndex, // finds index of a selected audio via .indexOf(...)
); I was really confused what went wrong. Decided to try the same thing on Android build of my app which uses non-modified
I've been testing this package on my own app that uses
Unfortunately, I have missed that "expects a static playlist" part. Ouch.
No, I did not. Tested the simple cases, with and without shuffle enabled, checked whether media gets re-downloaded where it is not supposed to.
I'm not sure if there is a way to tell just_audio to cancel/discard the media, so there isn't such a thing. P.S.: I really hope that this comment doesn't sound harsh or anything. |
Decided to try my fork with |
Any progress with that? 👀 |
Sorry for the long wait, not really, I have been also busy with university these past weeks. I've only been able to now merge some smaller PRs which didn't need a lot of checking. Anyways, I'll try to put a rush on this, and hopefully merge it soon enough. |
Same here, that's how this package should act.
Yes, I remember testing it with the example app, that's why I was a bit weirded out, I won't exclude that it might've been some misconfiguration on my part. If I manage to get to it I will also try testing it again.
Oh, nice! This means that we can also retain the prefetching functionality!
Don't worry, it doesn't, everybody makes mistakes, this time it just happened the mistake was mine 💀 Anyways, thank you again @Zensonaton for the very thorough comment and for your work on this PR, and I am really sorry it is taking so much time for me to merge this, but the time I can dedicate to open source work is very few... I'll try to rush this as much as I can though. |
Closes #3. All the details are explained in the #3.
Please thoroughly check the PR before merging this, there is a possibility I've made something wacky during conflict resolving :D