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

Spirc: Replace Mecury with Dealer #1356

Open
wants to merge 125 commits into
base: dev
Choose a base branch
from

Conversation

photovoltex
Copy link
Contributor

@photovoltex photovoltex commented Sep 29, 2024

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:

  • 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.
  • 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
    • update: seems to be resolved, was probably just an early development problem

What i still have to do:

  • handling the remaining request commands
    • queue handling (set_queue, add_to_queue)
    • repeat handling (set_repeating_track, set_repeating_context)
      • i didn't see these in the wild yet, so it might that they do not exists
    • context handling (update_context)
    • options handling (set_options)
      • should be related to shuffle and repeat
  • transferring the state to a different player again (stop playback if we are not active anymore)
  • 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
  • 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
  • autoplay has to be fixed
  • 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
Copy link
Contributor Author

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
Copy link
Member

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 😉

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

- impl shuffle again
- extracted fill up of next tracks in own method
- moved queue revision update into next track fill up
- removed unused method `set_playing_track_index`
- added option to specify index when resetting the playback context
- reshuffle after repeat context
@photovoltex
Copy link
Contributor Author

And with that the search result we resolve, should now expect the result from the search in each client. Additionally i could cleanup a workaround for the single track and autoplay context :D

Thx again for reporting the issues^^

@PocketMiner82
Copy link

PocketMiner82 commented Nov 27, 2024

Hey @photovoltex, the re-sorting of the liked songs is indeed working now. Playing a single song from search also works now. 👍

However:

  • if you try to click on a song from "Recent searches", it still does not play (this time with a different error message :P):
[2024-11-27T14:32:00Z ERROR librespot_connect::spirc] failed to resolve context 'spotify:search': Unknown error { Response status code: 502 Bad Gateway }
[2024-11-27T14:32:00Z ERROR librespot_connect::spirc] failed to handle request: Invalid state { context is not available. shuffle: Default }
  • Also, I do believe that when playing a song from search, that it continues playing other results from the search (which can result in the same song being played multiple times, e. g. from different albums or dozens of remixes) instead of creating an autoplay based on the selected song.

    You can test the behavior like this:
    While not connected to librespot, search for a song and click on it. the app says Playing from search\n"<what you searched>" in Search in the player view on the top:

    then skip to the next song. the app now says Playing recommended songs for you. Also have a look what songs are in the Next from queue:

    If you do the same thing while connected to librespot, the app will state Playing from search\n"<what you searched>" in Search even after you skipped to the next song:

    skipped to the next song:

    Also look what songs are in this Next from queue compared to the queue from above.

Maybe these two issues are related because I think you don't have a search context when trying to play from "Recent searches", idk.

I tested all of this only on Android.

@photovoltex
Copy link
Contributor Author

Also, I do believe that when playing a song from search, that it continues playing other results from the search (which can result in the same song being played multiple times, e. g. from different albums or dozens of remixes) instead of creating an autoplay based on the selected song.

I would actually say the behavior of librespot is not wrong. But yeah web-player and android seem to not actually play from the search context. Just checked what happens If we would run into autoplay for the search and it seems to fail. So I will rewrite it so that we always just play the track provided by a search. Seems easier anyways after implementing the other solution xD.

But mobile really is a special client for the dealer implementation...

- handle logout command
- disable support for logout
- add todos for logout
@photovoltex
Copy link
Contributor Author

I disabled logout support for now, because there is no proper logic in place to logout from a session. We could solve that afterwards, but for now it should probably stay disabled so it doesn't get in the way of merging these changes^^

@PocketMiner82
Copy link

very nice work @photovoltex! playing from recent searches works now. this PR also seems to fix #1205 then :D

just noticed one small bug introduced with the latest changes: if you play a song from recent searches or search, then everything looks fine, you can click the queue, shuffle and repeat buttons:
Screenshot_2024-11-28-09-10-49-540_com spotify music-edit
but when you skip to the next song, then the queue, shuffle and repeat buttons get grayed out and are no longer clickable but it is still possible to skip to the next or previous songs:
Screenshot_2024-11-28-09-20-24-914_com spotify music-edit

@photovoltex
Copy link
Contributor Author

That's an intended behavior because you switched to the autoplay context.

We could probably argue if it makes sense to disabled the queue (don't even know exactly why that happens). But I would argue shuffle and repeat don't make much sense when you are playing an infinite context.

@PocketMiner82
Copy link

PocketMiner82 commented Nov 28, 2024

That's an intended behavior because you switched to the autoplay context.

Ah, I didn't knew that. Also somehow did not notice it while testing xD

We could probably argue if it makes sense to disabled the queue (don't even know exactly why that happens). But I would argue shuffle and repeat don't make much sense when you are playing an infinite context.
I personally do think that it makes sense to not disable the queue button because you could/should be able to re-sort the autoplay queue.

The only reason I could come up with why repeat could be useful is that you would be able to repeat a single song that is in the queue.
I'd also argue that shuffle isn't needed for a (already random) context.

Also, the normal behavior of the Spotify app when not connected to librespot is to disable the repeat and shuffle buttons but leave the queue button enabled.
Now the question is: Do we want librespot to try to mimic the exact behavior of the official clients or do we want the additional feature to enable repeating of a song in the queue (if it is even possible to only enable the repeat button)? I'd argue it's better to try to mimic the exact behavior of the official clients because of consistency. What do you think @photovoltex?

@photovoltex
Copy link
Contributor Author

I would probably also agree to mimic the behavior. I mean that's why shuffle and repeat is already disabled :D. Will look later into it why the mobile version disables the queue. In the web-player you can do that at any point so I think it might be a mobile specific behavior again.

@photovoltex
Copy link
Contributor Author

photovoltex commented Nov 29, 2024

I adjusted the handling yet again on that part. It might now hopefully behave similar to the other clients.

Additionally i fixed the weird behavior when going to the previous track, without there being a prev track. There is a restriction set, that technically forbids the client to do it anyways... but mobile does seem to ignore it :D

CI failures seem to be related to the new stable release, apparently new clippy warnings that let the pipe fail :D

@PocketMiner82
Copy link

PocketMiner82 commented Nov 30, 2024

@photovoltex The behavior seems to be (almost) the same as on the official clients now - the buttons get grayed out except for the queue button. Very nice :D

I can also say now that the UI autoplay switch is also ignored when using the Android app. autoplay only works when forcing via --autoplay on. log when toggling the autoplay switch off and on again in the Android app:

[2024-11-30T08:50:35Z TRACE librespot_core::mercury] mercury response <spotify:user:attributes:mutated> is handled by dealer
[2024-11-30T08:50:35Z TRACE librespot_connect::spirc] Received attribute mutation for autoplay but key was not found!
[2024-11-30T08:50:36Z TRACE librespot_core::mercury] mercury response <spotify:user:attributes:mutated> is handled by dealer
[2024-11-30T08:50:36Z TRACE librespot_connect::spirc] Received attribute mutation for autoplay but key was not found!



Also, when autoplay is turned off, I see these errors when selecting a song from search:

[2024-11-30T09:01:41Z INFO  librespot_playback::player] <We Are The People> (267354 ms) loaded
[2024-11-30T09:01:42Z ERROR librespot_connect::spirc] failed to resolve context '': Requested entity was not found { Response status code: 404 Not Found }
[2024-11-30T09:01:42Z ERROR librespot_connect::spirc] ContextError: Invalid state { context is not available. shuffle: Default }

skipping to the next song then causes these errors (i think skip should be disabled when there are no songs left in the queue):

[2024-11-30T09:04:55Z INFO  librespot_connect::spirc] Not playing next track because there are no more tracks left in queue.
[2024-11-30T09:04:55Z ERROR librespot_connect::spirc] failed to handle request: Invalid state { context is not available. shuffle: Default }



it also seems like that the Android app loads the autoplay recommendations differently than librespot. The app loads the recommended songs after skipping to the next song. Librespot directly loads them when clicking on a search result. I think that these different behaviors cause the following bug: if autoplay is on, you already selected a song before connecting to librespot, did not skip to the next song and then connect to librespot, you get these errors after connecting:

[2024-11-30T09:06:31Z INFO  librespot_playback::player] <We Are The Champions - Remastered 2011> (179200 ms) loaded
[2024-11-30T09:06:32Z ERROR librespot_connect::spirc] failed to resolve context '': Requested entity was not found { Response status code: 404 Not Found }
[2024-11-30T09:06:32Z ERROR librespot_connect::spirc] ContextError: Invalid state { context is not available. shuffle: Default }

also, skipping to the next song does not work (because there is no queue available). this error gets logged:

[2024-11-30T09:07:55Z INFO  librespot_connect::spirc] Not playing next track because there are no more tracks left in queue.
[2024-11-30T09:07:55Z ERROR librespot_connect::spirc] failed to handle request: Invalid state { context is not available. shuffle: Default }
[2024-11-30T09:07:55Z ERROR librespot_connect::spirc] failed resolving context <context_uri: , resolve_uri: Some(""), autoplay: true, update: true>: Client specified an invalid argument { Response status code: 400 Bad Request }
[2024-11-30T09:07:55Z ERROR librespot_connect::spirc] ContextError: Invalid state { context is not available. shuffle: Default }

@photovoltex
Copy link
Contributor Author

I can also say now that the UI autoplay switch is also ignored when using the Android app. autoplay only works when forcing via --autoplay on. log when toggling the autoplay switch off and on again in the Android app:

The info is still retrieved via mecury. In the future we probably would want to switch to the ucs info retrieval. I also saw that behavior before, while testing but currently it does seem to send the data as expected.

Funnily enough, for myself autoplay sometimes is enable and sometimes not, even tho I didn't change if autoplay was enabled or not.

Also, when autoplay is turned off, I see these errors when selecting a song from search:

Ugh, that's not so good. Already got to reproduce it. Don't know why that didn't occur when i testing. Will look into it.

i think skip should be disabled when there are no songs left in the queue

Before these changes it was disabled, but it did feel a bit wrong to not be able to skip to the next track. I will adjust the logging messages accordingly.

it also seems like that the Android app loads the autoplay recommendations differently than librespot. The app loads the recommended songs after skipping to the next song. Librespot directly loads them when clicking on a search result.

Huh... I looked into it what the mobile client actually send... For the first played track from the search it sends 40 tracks all delimited by delimiter-tracks. So in total 80 tracks. Anyhow meaning we probably just need to add a delimiter and the mobile client should display our content as expected. I also tested if the mobile client without autoplay would maybe play the search-context, but no. It just stops after skipping to the next tracks.

- unify naming
- move more metadata infos into metadata.rs
- improved certain logging parts
- preload autoplay when autoplay attribute mutates
- fix transfer context uri
- fix typo
- handle empty strings for resolve uri
- fix unexpected stop of playback
@PocketMiner82
Copy link

@photovoltex now the recommendations load exactly the same as in the Android app. That means #1205 can now definitely also be closed when merging this PR :D

Unfortunately I noticed a new bug since the last update (which was not there before I updated):

  1. When clicking on a song from recent searches and then trying to transfer playback back to the Android app it does not work. When you then select another song, it suddenly transfers the playback to the Android app. If you don't do that, you are no longer able to control librespot (e. g. pause) until you restart the Android app.
    Log after selecting another song (after trying to transfer playback to the Android app):
[2024-12-01T08:23:36Z INFO  librespot_playback::player] Loading <Live Is Life (Live)> with Spotify URI <spotify:track:7fCjVnzKfFecNfJ745u3MH>
[2024-12-01T08:23:37Z INFO  librespot_playback::player] <Live Is Life (Live)> (465833 ms) loaded
[2024-12-01T08:25:49Z INFO  librespot_connect::spirc] device became inactive
[2024-12-01T08:25:49Z WARN  librespot_core::dealer] No subscriber for msg.uri: social-connect/v2/broadcast_status_update
[2024-12-01T08:25:49Z WARN  librespot_connect::spirc] failed filling up next_track during stopping: context is not available. type: Default
[2024-12-01T08:25:51Z WARN  librespot_core::dealer] No subscriber for msg.uri: social-connect/v2/broadcast_status_update
  1. When you pause before trying to transfer playback back, it enters (I think) the same weird state but this time you cannot resume. When you then select another song, it suddenly transfers the playback to the Android app.
    Log after selecting another song (after trying to transfer playback to the Android app):
[2024-12-01T08:30:19Z INFO  librespot_playback::player] Loading <We Will Rock You - Remastered 2011> with Spotify URI <spotify:track:4pbJqGIASGPr0ZpGpnWkDn>
[2024-12-01T08:30:19Z INFO  librespot_playback::player] <We Will Rock You - Remastered 2011> (122066 ms) loaded
[2024-12-01T08:30:40Z INFO  librespot_connect::spirc] device became inactive
[2024-12-01T08:30:40Z WARN  librespot_core::dealer] No subscriber for msg.uri: social-connect/v2/broadcast_status_update
[2024-12-01T08:30:40Z WARN  librespot_connect::spirc] failed filling up next_track during stopping: context is not available. type: Default
[2024-12-01T08:30:41Z WARN  librespot_core::dealer] No subscriber for msg.uri: social-connect/v2/broadcast_status_update

This seems to only happen when playing a song from recent searches, I could not reproduce it otherwhere (not even in the normal search).

- uid getting replaced by empty value
- shuffle/repeat clearing autoplay context
- fill_up updating and using incorrect index
@photovoltex
Copy link
Contributor Author

Funnily, that is one of the issues I encountered myself, but ignored it with the reasoning of "it's probably just a development state that will not happen in the wild :^)"

I tried a bit around, fixed some other incorrect states and behavior but didn't find a way to mitigate the issue regarding transferring the playback to the mobile app. I would suspect that it has to do with some incorrect state. Will try to find a solution the next days.

Big thanks again for all the testing and additional issue reports^^

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

Successfully merging this pull request may close these issues.

4 participants