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

Upgrade librespot to 0.2.0 #977

Merged
merged 13 commits into from
Dec 7, 2021
Merged

Conversation

capnfabs
Copy link
Contributor

@capnfabs capnfabs commented Jul 11, 2021

This is an initial attempt at upgrading librespot to 0.2.0. That also required:

  • upgrading tokio to 1.0
  • reworking dbus_mpris.rs to use dbus-crossroads.

The dbus-mpris stuff was all written using an older version of dbus-rs, which integrated with tokio 0.1, and therefore needed to be upgraded. I initially tried to do this with the dbus-tree package to minimise code changes, but I found it difficult to reason about the threading model, and eventually switched to dbus-crossroads. I hope that's ok!

I've left a number of TODOs in the code:

  • some are product-ish questions (e.g. "do we want to support this new librespot feature in the spotifyd config?")
  • some are places where the code works on my machine, but I'm unclear on whether it will work for everyone.

I'd love some input on how to resolve those!

Fixes #969

@busimus
Copy link

busimus commented Jul 23, 2021

I'm desperate for an upgrade to MPRIS so I tested this PR. Here's what I found:

  1. spotifyd doesn't show up in playerctl -a status as it did previously and running playerctl -p spotifyd play returns "No player could handle this command", even though spotifyd shows up in playerctl -l.
    I tried changing the object path in code from / back to /org/mpris/MediaPlayer2 like it was, didn't fix everything – playerctl commands started working but you still have to specify -p spotifyd or it will just ignore spotifyd and send the command to some other player. This used to work right.

  2. It still takes an unreasonably long time to query basic properties like PlaybackStatus, which for me takes up to half a second to respond (when using dbus-send). This used to slow down playerctl a lot and it still does. With the path fix mentioned above, running playerctl -p spotifyd play-pause takes up to an entire second.

Oh and also enabling volume_normalisation completely blows out the sound, but I'm not sure whose fault is it.

Would love to help getting any of this debugged if needed, because otherwise the MPRIS feature will remain unusable if you wish to control multiple players, not just spotifyd.

@capnfabs
Copy link
Contributor Author

capnfabs commented Jul 23, 2021 via email

@capnfabs
Copy link
Contributor Author

capnfabs commented Aug 1, 2021

Hey! I tested this briefly today. Here are the results:

  • Spotifyd doesn't show up in dbus until after a track starts playing. That's new as of this PR.
  • The DBUS rework appears to entirely not work on Linux? I'm not sure why, I tested this extensively on OSX.

I'll dig into it.

Haven't looked into the volume normalization yet; but I see log messages saying

This track will at its peak be subject to NaN dB of dynamic limiting.

Which sounds like it might be the culprit; I might've rigged something up badly.

@capnfabs
Copy link
Contributor Author

capnfabs commented Aug 1, 2021

Alright:

The DBUS rework appears to entirely not work on Linux? I'm not sure why, I tested this extensively on OSX.

The problem here was indeed what @busimus mentioned here:

changing the object path in code from / back to /org/mpris/MediaPlayer2 like it was

Re this:

playerctl commands started working but you still have to specify -p spotifyd or it will just ignore spotifyd and send the command to some other player. This used to work right.

@busimus, what do you mean by "right" in this case? I'm not super familiar with dbus, but my understanding is that playerctl chooses arbitrarily unless you have playerctld running. I'm not sure how playerctld monitors player status, but I could imagine it interacting badly with the way DBUS is implemented at the moment (doesn't send change signals, gives weird results if the API call gets rate limited, which happens somewhat frequently).

Final DBUS regression:

Spotifyd doesn't show up in dbus until after a track starts playing

This was something gnarly to do with futures / async/await that I don't quite understand, but I've added a hack to workaround it.

@capnfabs
Copy link
Contributor Author

capnfabs commented Aug 1, 2021

Alright, fix for normalization is in too.

@busimus
Copy link

busimus commented Aug 1, 2021

what do you mean by "right" in this case?

This was probably me forgetting that I wrote many workarounds to make it work right. I guess the dream is that playerctl -p playerctld play-pause command works fast and without additional hacks.
I tested everything again, the latest released version of spotifyd indeed does not work right: playerctld doesn't notice changes in activity of spoytifyd so it ignores it after it discovers another player.
With this PR, it ignores spotifyd completely, it doesn't show up in playerctl -l. Something I noticed when running playerctld in console is that it throws this warning when spotifyd starts:

(playerctld:322578): playerctl-WARNING **: 21:25:28.160: could not get properties for active player: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: Path, Interface, or Method does not exist

Alright, fix for normalization is in too.

I wish I could say I remembered to disable normalization when I started testing new changes before that commit. But it did fix it.

@capnfabs
Copy link
Contributor Author

capnfabs commented Aug 1, 2021

FWIW, I think the issues with play-pause being slow to respond are actually not in the DBUS implementation; it's got something to do with the way frames are buffered in ALSA. I've been using the Rodio backend on OSX and it's been very responsive, at least.

There are additional issues with other MPRIS methods requiring a server round trip -- notably, the metadata command. I've reworked that in master...capnfabs:dbus-event-plumbing to use events from librespot, but if you want to test it out, you'll also need this fork of librespot: librespot-org/librespot@dev...capnfabs:flopbox

@busimus
Copy link

busimus commented Aug 1, 2021

I'm using the pulseaudio backend and it stops fast enough if I use dbus-send. But playerctl -p spotifyd play-pause takes up to a second, orders of magnitude longer than any other player.

I tested that branch. Getting metadata or playback status is instantaneous but strangely enough playerctl -p spotifyd play-pause still takes 0.25 seconds.
Looking through dbus-monitor output I found that playerctl called the Properties.GetAll method for the player and it caused the slowdown. Here's that method call as a command:

dbus-send --print-reply --dest=org.mpris.MediaPlayer2.spotifyd /org/mpris/MediaPlayer2 org.freedesktop.DBus.Properties.GetAll string:org.mpris.MediaPlayer2.Player

So, if you manage to speed up GetAll and get playerctld to stop ignoring spotifyd with that warning, I think the dbus issue will be solved for good (given that all the changes get accepted into both projects of course).

@capnfabs
Copy link
Contributor Author

capnfabs commented Aug 1, 2021

Ah, I think I know what's going on -- GetAll would also fetch the loop status and repeat properties; and those (and maybe Position?) are currently still retrieved through the Spotify API.

Thanks for the repro command, that's super helpful! I think I'll open an issue with librespot and ask them if they've got ideas for fetching Loop/Shuffle status, but I'm also inclined to say "let's not worry about it for this PR and do it in the future given that it's not a regression."

@busimus
Copy link

busimus commented Aug 2, 2021

let's not worry about it for this PR and do it in the future given that it's not a regression

Yeah you're right.
I changed shuffle and loop status properties to return constants (because I don't care about them) and now everything is fast. Still requires a little workaround script to make it work seamlessly, but it's good enough for me to use daily, thank you.

EDIT: Or does it require a workaround? I compiled in release mode and suddenly playrctld doesn't throw any warnings and playerctl -p playerctld play-pause works perfectly with other players. After restarting spotifyd a few times I found that playerctld throws that warning only occasionally and then refuses to work with spotifyd. In debug mode it throws the warning every time. Very strange.

@busimus
Copy link

busimus commented Aug 3, 2021

I tested that debug/release mode race condition a bit. It happens in the exact same way both in this PR and in the other branch: in debug mode playerctld throws the warning and ignores spotifyd, in release mode warnings happen rarely and when they don't then playerctl works perfectly (but slowly in this PR).
In all cases playerctld tries to GetAll properties for two interfaces and it fails if spotifyd takes too long to set those interfaces up after calling conn.request_name(...) (so adding a sleep after it makes it reproduceable).
Moving conn.request_name(...) after cr.insert(...) seems to fix this, but I'm not sure whether it also breaks something else.

In case you need them, here are dbus-monitor logs for all cases:
spd_pr_debug.log
spd_pr_release.log
spd_branch_debug.log
spd_branch_release.log

Cargo.toml Outdated
url = "1.7"
xdg = "2.2"
librespot = { version = "0.1.5", default-features = false, features = ["with-tremor"] }
librespot = { version = "0.2.0", default-features = false, features = ["with-tremor"] }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spotifyd should depend on librespot's individual crates (e.g. librespot-audio or librespot-connect), since the librespot crate is a binary and has a lot of dependencies spotifyd doesn't use, like miow, getopts, rpassword and sockets2.

See #951

@jacksongoode
Copy link

jacksongoode commented Aug 12, 2021

This would be a huge update 🥳 thanks!

@f0rki f0rki mentioned this pull request Sep 27, 2021
6 tasks
@JockeTF
Copy link

JockeTF commented Oct 21, 2021

I've been using this branch for a little while now. It works very well.

@robinvd robinvd requested review from robinvd, JojiiOfficial and a team October 21, 2021 13:47
@robinvd
Copy link
Contributor

robinvd commented Oct 21, 2021

Amazing update! its a little much code to review right now, but ill try and test it this week. Eventhough it introduces some TODO's i think we should merge and maybe see to those at a later time

robinvd
robinvd previously approved these changes Nov 12, 2021
@capnfabs
Copy link
Contributor Author

@robinvd thanks for testing!

Just pushed a commit to use librespot libraries directly, which address @hhirtz's feedback above.

I think this is ready to go from my side, but I don't know what still needs to be done. Do you have any input / feedback about next steps?

@robinvd
Copy link
Contributor

robinvd commented Nov 19, 2021

We now just need 2 maintainers to approve the pr

robinvd
robinvd previously approved these changes Nov 19, 2021
hhirtz
hhirtz previously approved these changes Nov 19, 2021
@capnfabs capnfabs dismissed stale reviews from hhirtz and robinvd via d2d00d4 November 19, 2021 11:11
@capnfabs
Copy link
Contributor Author

Hmm, still a bunch of lint checks / CI Failures; I'll see if I can work through them this weekend.

@capnfabs
Copy link
Contributor Author

Alright, after a bunch of messing around, I've finally got all the checks passing 😅

There's one change I was a little suspicious about -- changing the capitalisation of TV, AVR, and STB in f86275a to make clippy happy. I tested that with a config file on my local machine, and the values still deserialize correctly.

@MDeiml
Copy link
Contributor

MDeiml commented Nov 20, 2021

Something I noticed when I worked with your changes: I think you can make the rspotfy dependency optional and only use it with the dbus_mpris feature.

@SimonTeixidor
Copy link
Member

Thank you for this work! This looks pretty good to me. A few TODOs left, but I agree with @robinvd, they can be dealt with later. I'll go ahead and merge.

@SimonTeixidor SimonTeixidor merged commit d10216a into Spotifyd:master Dec 7, 2021
@SimonTeixidor
Copy link
Member

Ouch, I only reviewed the code, and missed the 13 commits. In retrospect, we probably should have squashed those before merging... :)

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.

Tracking issue for upgrading librespot to v0.2.0
8 participants