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

Draft for playon compatibility with the jellyfin server #850

Draft
wants to merge 19 commits into
base: redesign
Choose a base branch
from

Conversation

pinsarda
Copy link
Contributor

@pinsarda pinsarda commented Aug 25, 2024

This is a very early shot at getting the playon feature from jellyfin to work with finamp.
For now this is a extremely rough draft to start working on the feature and getting rid of an annoying bug. Can only start songs without having media controls and need to enter the url manually.

@Chaphasilor
Copy link
Collaborator

Thanks for getting this started, really looking forward to it!

So I managed to get this working a bit more reliably and generically some time ago, just pushed those changes :)
The playback controls are now all shown, and Finamp registers support for all commands I could find. The important ones are already implemented too.
I didn't work on the actual "play on" functionality though, so that's unchanged, meaning it works for the most part, but sometimes the wrong item is picked (e.g. selecting a specific track from an album will play the first track from the album instead). Ideally, this should work just like it works in Finamp, and make use of the same settings. So, if a single track is selected, depending on the Finamp settings it should either play just that track or start an instant mix from that track.

There are still issues with the websocket, it seems to get disconnected (or at least stops receiving messages) after a short while or certain commands. Haven't looked into that just yet.
I also noticed when testing this out again that when logging into the server the websocket connection will fail until the app is restarted, because the connection is established before the URL and credentials are know. The initialization should therefore be delayed until setup is complete, or restarted after successfully logging into a server.

@pinsarda
Copy link
Contributor Author

I apologise for taking so much time to go back to this, but as they say better late than never ! I didn't find the source of the issues with the websocket, still investigating. For now I quickly fixed the item selection, now it starts the instant mix and the correct song as it should.

@pinsarda
Copy link
Contributor Author

The current implementation works very well on my phone, but can probably be a bit slow as I request each song individually instead of the whole album at once. I don't know if there's an easy way to do it better currently...

@Chaphasilor
Copy link
Collaborator

Well just take a look at how the content of an album is fetched for the album screen :)
It's just a matter of setting the parentID filter, I think.

@pinsarda
Copy link
Contributor Author

Yes I thought about that but this would introduce a bug when starting a playlist. The websocket just gives us ids of songs to play. It doesn't ask finamp to play an album or a playlist. This makes it hard to do it differently.

@Chaphasilor
Copy link
Collaborator

Is there no way at all to differentiate from "where" the track was selected? I guess in that case all we can do is play just that one track (or start the instant mix, depending on settings), and nothing else.
Is that also the case when you try to play an entire album or playlist? Does it only send us the first track, without any additional information?

@pinsarda
Copy link
Contributor Author

I don't think so, we just receive a list of ids, and optionally the index of the track selected if there was any. The current implementation does what you're proposing. If a specific track is selected, we only play this one or start instant mix. And if there's no precision on the starting index it queues the whole album/playlist. After testing it seems like the best possible behaviour to me.

@pinsarda
Copy link
Contributor Author

As discussed in #500 an internal volume controller will be implemented eventually, we'll rely on that instead of system wide volume.

@Chaphasilor
Copy link
Collaborator

Chaphasilor commented Sep 21, 2024

@Maxr1998 how did you approach this issue (playing albums/playlists) on the Android (TV) app? 🤔

@Maxr1998
Copy link
Collaborator

Can't speak for Android TV, not even sure play on is supported there. On the Android app, we just wrap the web app so the handling is identical to that.

@Komodo5197
Copy link

This seems like the server is giving us a list of song IDs to form the queue, which means the most similar piece of code is loadSavedQueue() in lib/services/queue_service.dart, so you can probably look at that for inspiration. Most notably, you can make batch requests for up to 200 item IDs at once, which is significantly faster. Also, I'm pretty sure the behavior of loading the single song or an instant mix is questionable. We should be loading the whole id list from the server into the queue and just setting the initialIndex to what the server tells us to.

@Chaphasilor
Copy link
Collaborator

Chaphasilor commented Sep 22, 2024

@Komodo5197 the playing of a single track should only happen when the user selects a single track in the other client, and the API only gives us one ID. If we get multiple IDs, we should play all of them of course.

Thanks for the suggestion about the queue restore, that would make sense.

However, my main concern was how to identify where these tracks are coming from, so we can show a proper queue source. If the user requests to play an album, I'd like Finamp to show which album is playing as the queue source name (so that one could also tap the source at the top of the player screen, and immediately go there). Same for playlists, artists, etc. But it seems that info is simply not given to us, so we'll have to default to some generic ("Queued by Remote Device"?) name instead.

@Komodo5197
Copy link

We are getting the full queue. I click a track partway through an album on the web UI and finamp recieves {"MessageType":"Play","MessageId":"560c155a593a497e8bd047610f2e8e4a","Data":{"ItemIds":["46430779c8072941ed8f7e2ddafaebe4","f8ee032bb75db53c1c317c572e3e8814","fda9dba1e21b261bdb050efe5bd95bbb","4b0a4b5c756079eb8322860215fbe5fb","e012de5b9fb821a2258b6e91c9225459","ea8e6422f6266c453469e104e3e4b950","cbb922f4f858571b79dafa8da3578270","d891ee5acb57e58ca338df5518c8ce37","80ed7b24b4878c28a75e6234ff066bd6","ca4e6f99c7ad2bae487aaba0585f869c","d586e2361bab084b04872a4128507db8","67f19176d6582739abfd0678442b45de","c34b8e312fd99b7194d927c64a9700a9","c89368641479249da0b333c09ba1aa40","ba8ade9d002d8c40bae852531bcebba5","8ac7aa4480fa7fcaecf1d03a816431ed","22a0587b3018e44644924535542ace41","ca63bf02ba1a0bee484e2fb6e036d3e0"],"PlayCommand":"PlayNow","ControllingUserId":"de6a683bafb44247881bb62bf125f7e8","StartIndex":8}}.

There are also additional play commands 'PlayNext', corresponding to addToNextUp(), and 'PlayLast', corresponding to addToQueue(). Implementing those allows remote queue building that seems to match up with how the web client works.

Regarding queue source, it does indeed seem that we don't have this information in any form, and will need to use a generic header.

Also, I happen to still be running a 10.8 server, and to get this to work I had modify the ClientCapabilities to explicitly set supportsSync and supportsContentUploading to false, and remove the supported command "SetPlaybackOrder".

@Chaphasilor
Copy link
Collaborator

Okay, thanks for taking a look. Haven't had a look at the code recently, but there are still instances where a single track is selected, e.g. when playing from a search result. In that case, the Instant Mix behavior should still apply, I think.

Interesting note about the 10.8 server. I also still have one of those lying around, I'll give it a try myself. Maybe the commands had a different name back then? I'd like to keep the playback order functionality if possible, but I'd also like to keep supporting 10.8.
Maybe it's finally time to query the server info (version, name, etc.) on startup, and have some version-specific features. I guess feature detection is a hopeless endeavour with Jellyfin anyway...

@Chaphasilor
Copy link
Collaborator

I gave the current version a try yesterday. It was working well at first, and I could see the queue, playback info, and control playback mostly fine. But later on I didn't manage to get the queue or even the playback info to show up anymore, even after restarting the app.
I also encountered an issue where it seems like commands where being queued for some reason. I tried playing/pausing and sending a few other commands (including a message) through the web client to Finamp, which didn't have an effect. But then many minutes later I tried to pause playback (in Finamp directly), but it was resumed immediately. That happened a few more times, and then finally it actually did pause and stayed paused, and then the message appeared. Maybe there's a command waiting for a Future that never resumes, or something similar?

@pinsarda
Copy link
Contributor Author

For the first issue, it's weird I didn't experience it. I got the same bug regarding command queuing though, and got to the same conclusions as you without nailing it yet. I'll get back to it tomorrow and on thursday. I'll also adjust the code to have the behaviour proposed by @Komodo5197

@pinsarda
Copy link
Contributor Author

@Chaphasilor Couldn't spend as much time as I wanted on this, but as we guessed, using unwaited instead of await when executing the commands (expecially Pause, Unpause and PlayPause) completely fixes the issues I had with unresponsiveness after sending too many commands, and also makes everything smoother. Is it a bad practice to do so ?
It definitely needs some more testing to be sure but now it seems to "just work" on my device.
There's one remaining bug when seeking I have to fix. Jellyfin takes a bit of time to update the progress bar on the web client, because finamp doesn't report playback state directly.

@Chaphasilor
Copy link
Collaborator

Hmm. It's good that we seem to have found the issue, but the best idea is probably to figure out why the play/pause futures don't resolve like they should.

Aside from that, will the websocket reconnect to the server after changing i.e. from WiFi to mobile data?

@pinsarda
Copy link
Contributor Author

pinsarda commented Sep 27, 2024

Ok, I'll check further, I didn't see anything suspicious last time I checked the futures.
With my current setup (VPNs and stuff) I unfortunately can't check easily if the websockets reconnect when changing network, but I highly doubt it. Disconnecting and reconnecting wi-fi closes the connection. Changing this, as well as fixing the bug when login in for the first time, is on my to do list

@Chaphasilor
Copy link
Collaborator

Just wanted to note that the websocket connection should get disconnected when switching to offline mode, before I forget about it.

@pinsarda
Copy link
Contributor Author

pinsarda commented Nov 6, 2024

There's an issue when using playlist, the songs are completely unordered. This is an upstream bug from jellyfin : jellyfin/jellyfin#8885
I didn't look into the details just yet

@Chaphasilor
Copy link
Collaborator

That's because the server sends us the raw track list instead of just the playlist ID, right? If there is an ID / some way to figure out which playlist is being requested, be could simply disregard the track list and fetch the playlist ourself?

@pinsarda
Copy link
Contributor Author

pinsarda commented Nov 7, 2024

Absolutely, but the playlist id is not in the message received by the websocket unfortunately. Plus you have no way to even know if it's a regular album or a playlist being played (apart from checking if all the songs belong to the same album after fetching data). As you guessed, we only get a list of ids of songs to play and a starting index, that's it.
I have dirty workarounds in mind but I would rather have the bug fixed properly jellyfin side. And/or have the playlist/album id supplied directly in the websocket data, which would also be nice to populate the "playing from" label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants