Spirc: Replace Mecury with Dealer

[photovoltex]

As suggested from @roderickvd a draft PR so i can have some community feedback and give now and then updates on the progress.

The replacement is in a working state, so the very basic do work again. There is still a lot of cleanup and re-implementing to do.

But so far what is working:

• transferring the playback so that we are the player
  • funnily enough, transferring back isn't working yet
• play, pause, resume, prev, next, seek
• just general communication of our state to other clients

Known Problems:

• [x] on the initial connection librespot doesn't appear, but if for example the volume is slightly updated, we should appear

  this might be a web-player specific problem, in the client the dealer just pops up

  • this seems to be to some degree fine? if the devices are reopend it seems to be always there, it just isn't dynamically updated.
• [x] if the session or spirc/dealer dies, we lose our complete ConnectState
  • it would be nice if that is preserved so we could start from that again
• [ ] sometimes other client's forget that we are the current player
  • might be that that is already resolved, but i think i still saw that behavior not so long ago

What i still have to do:

• [ ] handling the remaining request commands
  • [x] queue handling (set_queue, add_to_queue)
  • [ ] repeat handling (set_repeating_track, set_repeating_context)
  • [ ] context handling (update_context)
  • [ ] options handling (set_options)
    • should be related to shuffle and repeat
• [x] transferring the state to a different player again (stop playback if we are not active anymore)
• [x] resolving the context from a cold state
  • ~~currently it works really well if the playback starts from a different device like the web-player~~
  • ~~but from a cold start it's still a bit weird and not fully sorted out~~
  • we don't copy the player state from a running instance anymore,
  • so that we actually use the the cold state resolving/handling/transfering
• [x] shuffling has to be re-implemented
  • we can set restrictions for the clients getting our state
  • so for autoplay we should ignore shuffeling and just disable repeat and shuffle
• [ ] i added the option for track repeat, but it's not handled yet
• [ ] i would like to remove the remaining mercury calls
  • but for that i will have to see if the dealer sendes equivalent messages/requests

I tried to keep my code clean, but if there are points that i could improve and such, i would like to here them^^

[photovoltex]

oh the branch might not compile because there are still some todo!() here and there which result in unreachable statements that clippy, understandably, doesn't like.

[roderickvd]

Super job! I know you're not there yet, so let's all contribute here. Stuff like this almost makes me sign up for a Spotify plan again :wink:

I'm gonna have a more thorough look later. Wanted to ping back on your comment over at #1349:

The hm://pusher/v1/connections/ message is send after connecting to set up some connect stuff initially. Can be safely ignored as far as i know. It just falls under one of the many unknown subscriptions category that are logged.

If you look at librespot-java, you'll see that it listens to this message to extract the Spotify-Connection-Id from the headers:

https://github.com/librespot-org/librespot-java/blob/d0ff31b4f476590c9c17401aac01a6762e4ba908/player/src/main/java/xyz/gianlu/librespot/player/state/DeviceStateHandler.java#L162

I think if you follow the code flow, updating the connection ID then triggers the ready event in the deviceStateListener, which in turn gets the volume. And that seems to gel with you saying that:

on the initial connection librespot doesn't appear, but if for example the volume is slightly updated, we should appear

[photovoltex]

I did recently go through old issues to see what this PR might fix. Following issues might we fix after merging these changes:

• #19
  • repeat track is an own state in the connect state context
  • and i will probably adjust the repeat handling to behave like the web-player dealer
• #434, #442
  • this should also be fixed, but that's related to resolving the context and i currently push that back because it will require a bit more time to figure out how we want to resolve that correctly
• #676
  • sometimes i have the behavior that we don't disappear, but usually, when the web socket is closed we also disappear in the ui
  • might still have to investigate why we sometimes don't disappear tho
• #701, #861
  • i just implemented the set_queue command, which does exactly resolve this problem
  • set_queue is also used to remove all queued tracks from the queue
  • but for adding an item to the queue we receive the add_to_queue command

[roderickvd]

Thanks for sifting though the issue list. I agree, with this new infrastructure in place we'll stand a real chance of fixing those and perhaps more issues. A lot were related to stuff missing on spirc.

I said it before, but mega job man. This is going to be awesome.

[photovoltex]

So... if you don't use repeat, play unavailable tracks, or have any other edge cases, this should be a quite stable state, in terms of no hard bugs and crashes (except maybe an integer underflow that might happen if you request skip_prev too much).

The current collection handling is the same as in all playlist. But it seems like spotify handles the collection differently and the index of the current track seems to be always at 10... will see if i change that because that handling seems kinda unintuitive.

While testing i also still had some weird behaviors where i had offsets where there shouldn't be any. Did happen in my liked songs. where at around 5k i had an offset of 25, around 100 3 and at the beginning none. I think it might have to do something with unavailable songs... But technically we have all songs locally loaded as context... Will investigate that probably next time.

Next up will probably be repeat, a bit more stabilization and then autoplay. The might actually still work, but i would like to adjust them to the new logic, or at least test them and fix probelem^^;

[photovoltex]

Some updates for stability. Mostly reconnecting should now work (it actually doesn't reconnect, it just picks up the transfer-state that is given to use from the first connect-state update) and updating the volume now happens with a delay of 2 seconds. Additionally the context is back to being resolved asynchronously because for a smooth transfer it was a really bad idea to resolve the context synchronously.

There still seems to be some problems here and there. Sometimes completely losing our playback on the transfer (seems to be sporadic... yay) and a weird offset problem with the position of the track, or sometimes playing the prev track if the playback is transferred around the track transition.

[roderickvd]

Curious: when shuffling is enabled, does Spotify provide the shuffled list of tracks? Or do we need to shuffle ourselves, "broadcast" the shuffled queue, and potentially un-shuffle again when requested?

I remember librespot-java had this shuffle implementation with a reversible randomiser.

[photovoltex]

Curious: when shuffling is enabled, does Spotify provide the shuffled list of tracks? Or do we need to shuffle ourselves, "broadcast" the shuffled queue, and potentially un-shuffle again when requested?

I remember librespot-java had this shuffle implementation with a reversible randomiser.

Currently the implementation just resolve the complete context. And when shuffle is toggled, it shuffles the complete context and store it separately to the normal context (would be nice if that could be done by references, currently i just clone the tracks). By that we have a context that we can just use replacement to the default context. Will look how librespot-java (or go-librespot) does solve the shuffle. But i kinda like the current solution, because it just changes the underlying context and continues playing from that.

[photovoltex]

I remember librespot-java had this shuffle implementation with a reversible randomiser.

Yeah librespot-java does it that way. I would probably stay at the current solution, unless reversible shuffle is preferred or a better solution, and just figure out how to only hold the refs in the shuffle context. By that we only have to shuffle once every time it gets enabled.

[roderickvd]

I remember librespot-java had this shuffle implementation with a reversible randomiser.

Yeah librespot-java does it that way. I would probably stay at the current solution, unless reversible shuffle is preferred or a better solution, and just figure out how to only hold the refs in the shuffle context. By that we only have to shuffle once every time it gets enabled.

No preference. If this works, all good to me!

[photovoltex]

good god... autoplay and all the possible states is something else... I really have to revisit most of the code, because i pretty much lost the overview what is where in place. So far it does work quite decently tho.

There is only one weird thing that i couldn't solve for now. After playing all 50 autoplay tracks (or just skipping), the next autoplayed context that we retrieve, seems to be nearly identical. So there might be a missing understanding from my side what the endpoint want's from me. Will probably look into it again the next days.

[photovoltex]

So, now we also handle repeat context and track. The logic is based on how spotify does things. I will probably have to document all the new state handling way more then now. And i would also like to go over the code and split it up into more files, currently we just have two ~1k+ files, which is fine in itself, but i find it a bit overwhelming if for example one only wants to adjust a smal section.

So yeah, the next steps will be stabilizing, testing, documenting and a whole lot of cleanup :D

[roderickvd]

So you're saying it's feature complete? 👀 🤩

[photovoltex]

I would think so? When i cleanup, i will probably check again. But the main features, in addition to track repeat, should be there and working.

There are probably still some cases that are not handled yet. For example, i only tested with an android device, the web-player and an linux desktop client. As android did send some other commands then desktop and web, there might still be some other commands that aren't covered yet.

Btw. there is also still some weird behavior around the position in the web-player that seems to come from updates without a position update from our side (for example changing the volume). Desktop and mobile seems to handle it fine, but the web-player seems to be very picky.

[photovoltex]

So i found a small road blocker, some requests require a uid to be available. But the resolve-context endpoint (that i now use to resolve the context) doesn't seem to send the uid when requesting for album or the collection (playlists work). The uid doesn't seem to be much of relevance for desktop or web. But mobile sends for the play command a uid to skip to. Because the "new" context endpoint doesn't send the uid we can't skip to a specified uid... if anyone has some info around the uid i would be happy to hear how we can retrieve it maybe form the default id.

I already tried it with apollo and that seems to send the uid, in addition to more metadata. But apollo resolves more then the requested context and seems to be limited at 50 tracks. Currently i like the resolve-context endpoint, because it does only resolve the requested context, and resolves it completely (it doesn't seem to have a limit, or at least it does resolve a context of 5k tracks). I will probably also look into some other requests that the official clients do, so that we could maybe use a different endpoint to resolve it on another way.

[roderickvd]

Unfortunately I have no experience with that. Best advice I can offer is to read through the librespot-java or maybe librespot-go source code to see how it's done there.

I agree that the Apollo station is too limiting.

[photovoltex]

So, just added the update of the context and playlist modification to the logic. It should work fine, but i had some cases where it behaved a bit weird. So might adjust some stuff the following days.

Besides that i only have three todo left in the code:

• two in regards to replacing the mercury subscriptions for: spotify:user:attributes:mutated and spotify:user:attributes:update
  • if anyone knows how to trigger them, I would much appreciate a info on that, otherwise i will probably just remove these todos
• and one for the reconnecting of the player state

Info's about the reconnecting todo: Currently when the session or dealer dies, everything is restarted. This usually isn't much of a problem, but when playing a shuffled context, it feels weird because the queue is also reshuffled. So our previous tracks we saw in the queue, are just being replaced out of nowhere. To fix that behavior i just have to handle the send context when connecting, it's easier to just ignore it, because it is usually isn't the entire context, but for this specific case it might be a good idea to handle it.

I use the current version quite regularly and for my use case it seems to work very well. Because I use bluethooth headphones it's a bit of a pain, because librespot stops working with the headphones when disconnecting and reconnecting. But the dealer aspect with desktop, web and mobile seems to work fine.

[roderickvd]

• two in regards to replacing the mercury subscriptions for: spotify:user:attributes:mutated and spotify:user:attributes:update

  • if anyone knows how to trigger them, I would much appreciate a info on that, otherwise i will probably just remove these todos

• and one for the reconnecting of the player state

I think you can trigger at least one of them by e.g. toggling normalisation on and off while you're already connected.

Info's about the reconnecting todo: Currently when the session or dealer dies, everything is restarted. This usually isn't much of a problem, but when playing a shuffled context, it feels weird because the queue is also reshuffled. So our previous tracks we saw in the queue, are just being replaced out of nowhere. To fix that behavior i just have to handle the send context when connecting, it's easier to just ignore it, because it is usually isn't the entire context, but for this specific case it might be a good idea to handle it.

For the first pass I'd say: no worries to ignore it. I mean, either the session or dealer dying seems like a pretty rare and critical event that would also warrant resetting the playlist on the client end.

It's great that you have some ideas to put the icing on the cake, very welcome, but also later if you want.

I use the current version quite regularly and for my use case it seems to work very well. Because I use bluethooth headphones it's a bit of a pain, because librespot stops working with the headphones when disconnecting and reconnecting. But the dealer aspect with desktop, web and mobile seems to work fine.

🚀

[roderickvd]

Give us a ping when you feel you're ready to go for code review.

[photovoltex]

Hmm, okay. Will try out the new changes for about a week and look into replacing the last mercury events the following days. So around next weekend i might put the pr out of the draft state. Might also make some final cleanups to the connect state because there are still some methods that without context probably are hard to distinguish what they actually do.

And thanks for the fast response and hint on the mercury subscription. I will update here again once I'm finished to a degree where we could merge it^^

[photovoltex]

Just found a part of my code that handled the updated of the context incorrect, which turned out to break the playback of artists. I put a quick fix on it, which just resolves the problem for now. But i will have to revisited a portion of the code and adjust it accordingly to handle multiple pages before falling back to autoplay or stopping the playback.

[photovoltex]

So, this might be a state that is worthwhile reviewing.

In a previous version i already replaced the attribute mutation and updates that were still related to mercury. So mercury is now completely replace with the dealer for the Spirc.

The latest updates just revisited the initial take-over handling. I did also look into the transfer error in regards to multiple librespot instances mentioned by @YutongGu in #1394. But that seemed to be fixed? Or at least i couldn't reproduce it anymore. Besides that there was an annoying bug where a mobile device just stole the playback, which should also be fixed.

I left a single todo (the before mentioned reconnect/shuffle todo) and a fixme where it technically works, but is just a workaround cause of missing knowledge what exactly the web-player requires to behave correctly.

I might still make some changes to the branch over the course of the week if i find some bugs that can be easily fixed.

Already massive thanks for anyone reviewing the code changes in advance, because they kinda escalated just a bit ^^;

[PocketMiner82]

Hey @photovoltex, I tested out your PR and I must say that it looks awesome, so many bugs fixed. I (currently) only found two bugs:

1. search for a song using the normal search while connected to librespot. tap on the song to play. it does not play and the following error is logged:

[2024-11-24T18:05:56Z ERROR librespot_connect::spirc] couldn't handle connect state command: Invalid state { could not find track None in context of 20 }

2. try to move a song in the Next from: ... queue. if you do so, it will get moved but the old position will get replaced with the moved song. E. g.: Next from queue is A, B, C, D. Drag C to the beginning. Now the next from queue should be C, A, B, D. But it actually is C, B, C, D.

[b-nobis]

Hi, I also tested your PR in the context of the MuPiBox project. There we use the Spotify web api to play on a librespot connect device. If I select an album or a playlist through the web api I get the following errors:

WARN librespot_connect::spirc] unknown request command: {"context":{"uri":"spotify:album:1Wstoinsm4CGJ4ybpWg8iH","url":"context://spotify:album:1Wstoinsm4CGJ4ybpWg8iH"},"endpoint":"play","logging_params":{"device_identifier":"webapi-xxxx"},"options":{"seek_to":0,"skip_to":{"track_index":0}},"play_origin":{"device_identifier":"webapi-xxxx"}}

WARN librespot_connect::spirc] unknown request command: {"context":{"uri":"spotify:playlist:57ta9SFJHUNCemuhm2ncEM","url":"context://spotify:playlist:57ta9SFJHUNCemuhm2ncEM"},"endpoint":"play","logging_params":{"device_identifier":"webapi-xxx"},"options":{"seek_to":0,"skip_to":{"track_index":0}},"play_origin":{"device_identifier":"webapi-xxx"}}

[photovoltex]

First of all, thanks for testing the branch :)

search for a song using the normal search while connected to librespot. tap on the song to play. it does not play and the following error is logged:

I can't reproduce the bug anymore. Tho, I know that there was a bug previously which blocked playing single tracks. This was fixed in https://github.com/librespot-org/librespot/pull/1356/commits/ac10e63163b3cf1f50bb00f50ad865b0a22df23e. So maybe just update your branch and the problem might go away.

If not, could you please provide a more detail explanation how you produced the incorrect state? The device (desktop, web, mobile) make a big difference (platform is probably unrelated). Especially mobile does things quite differently then web or desktop.

try to move a song in the Next from: ... queue. if you do so, it will get moved but the old position will get replaced with the moved song. E. g.: Next from queue is A, B, C, D. Drag C to the beginning. Now the next from queue should be C, A, B, D. But it actually is C, B, C, D.

~That I'm aware of, sadly. One of the pain points that are out of our control after we published our connect-state. If you try that example in the desktop or web-player it works without problems, but mobile does something else with it, which results in the messed up state of the queue. I already tried to figure out why it behave like it does, but couldn't find a solution for it, yet.

The problem with every around that is, that it is updated via the set_queue command. So we just apply what the command sends us. So there is something not quite correct with the state for the mobile client i presume.~

Scrap that, I think i just forgot to add the "queue-uid" on the set_queue command. Because it works, when the items are added via the web-player, so that might actually be an easy fix.

[photovoltex]

First of all, thanks for testing the branch :)

There we use the Spotify web api to play on a librespot connect device.

Hui... that I didn't test at all so far. In the provided case, their seems to be a missing field in the our request command defined of the play endpoint. I will try all player endpoints and add the remaining fields, so that the commands are de-serialized correctly.

Thanks for the reminder that the API exists^^. I totally forgot that there is another "client" besides web, desktop and mobile

[photovoltex]

The add from mobile should now work correctly and afterwards moving these tracks inside the queue shouldn't remove/overwrite tracks anymore.

I checked all web-api calls for the player and it seems that only the "Start/Resume Playback" had a problem with the PlayCommand model. In addition the seek_to field should now also be applied (seems to be web-api specific).

[PocketMiner82]

First of all, thanks for testing the branch :)

search for a song using the normal search while connected to librespot. tap on the song to play. it does not play and the following error is logged:

I can't reproduce the bug anymore. Tho, I know that there was a bug previously which blocked playing single tracks. This was fixed in https://github.com/librespot-org/librespot/pull/1356/commits/ac10e63163b3cf1f50bb00f50ad865b0a22df23e. So maybe just update your branch and the problem might go away.

If not, could you please provide a more detail explanation how you produced the incorrect state? The device (desktop, web, mobile) make a big difference (platform is probably unrelated). Especially mobile does things quite differently then web or desktop.

try to move a song in the Next from: ... queue. if you do so, it will get moved but the old position will get replaced with the moved song. E. g.: Next from queue is A, B, C, D. Drag C to the beginning. Now the next from queue should be C, A, B, D. But it actually is C, B, C, D.

~That I'm aware of, sadly. One of the pain points that are out of our control after we published our connect-state. If you try that example in the desktop or web-player it works without problems, but mobile does something else with it, which results in the messed up state of the queue. I already tried to figure out why it behave like it does, but couldn't find a solution for it, yet.

The problem with every around that is, that it is updated via the set_queue command. So we just apply what the command sends us. So there is something not quite correct with the state for the mobile client i presume.~

Scrap that, I think i just forgot to add the "queue-uid" on the set_queue command. Because it works, when the items are added via the web-player, so that might actually be an easy fix.

$ target/release/librespot --help
librespot 0.5.0 dab0948 (Built on 2024-11-24, Build ID: C54gdSQ6, Profile: release)

I already was on the latest commit when I wrote that, unfortunately. I updated to the now latest commit, both issues still exist for me. I tested both using the Android Spotify app. The queue still gets mixed up and I still get the same error message... I can also try a different client tomorrow just to verify whether it is just a problem with the Android app, if that helps.

Note that re-sorting a "manually" created queue works for some reason. Only re-sorting the tracks under Next from: ... (e.g. Liked songs) produces that weird behavior.

[b-nobis]

Now it also works with the web api 👍 But I see a lot ERROR librespot_connect::spirc] could not dispatch connect state update: Resource has been exhausted { Response status code: 429 Too Many Requests }.

[photovoltex]

@PocketMiner82

Note that re-sorting a "manually" created queue works for some reason. Only re-sorting the tracks under Next from: ... (e.g. Liked songs) produces that weird behavior.

That was a very good hint. With that i could reproduce the bug. I adjusted some stuff in regards to that. Maybe try playing a different context and try then to reproduce the bug again. I couldn't reproduce it anymore so it might be fixed now.

I can also try a different client tomorrow just to verify whether it is just a problem with the Android app, if that helps.

That might solve your problem. But probably doesn't solve the underlying issue. So just use it as a workaround not a solution^^