MPRIS Device Name

[klay2000]

Added a compilation option to append device names to the end of dbus paths, this allows multiple Spotifyd daemons to be running each with a unique dbus object for MPRIS as long as they have unique device names.

[eladyn]

Hi and thanks for the contribution!

While your approach certainly works, I think that it'd be much nicer, if this setting either were the default or customizable at runtime via some configuration option. The reason for this is that most people probably download a prebuilt spotifyd binary rather than building it themselves, so they never get to choose the dbus name behaviour.

Happy to hear your thoughts on that!

[klay2000]

A configuration option sounds like a good idea, I'll give that a try.

[eladyn]

Generally looks good to me now, thanks! You'll need to run cargo fmt before this is ready, to keep coding style consistent.

In the docs, there's also the sample config, where this new option should be added as well, accompanied by some short explanation.

[eladyn]

One last comment. Apart from that, I think this is ready. Thank you! Now, we'll just have to wait for the maintainers.

[JasonLG1979]

The spec says that apps that allow multiple instances should follow this convention.

So if the name is "spotifyd" the bus name should be:

org.mpris.MediaPlayer2.spotifyd.instance{deviceName}

Where deviceName is the sanitized device name. Bus names can only contain ASCII characters (but no -)

[JasonLG1979]

Really the easiest way is just to follow the spec and make the bus name org.mpris.MediaPlayer2.spotifyd.instance{pid} all of the time and not even use the DeviceName at all.

[eladyn]

The spec says that apps that allow multiple instances should follow this convention.

Thanks for chiming in! This is indeed something that should be considered.

So if the name is "spotifyd" the bus name should be:

org.mpris.MediaPlayer2.spotifyd.instance{deviceName}

Where deviceName is the sanitized device name. Bus names can only contain ASCII characters (but no -)

If the deviceName doesn't start with a number, we could omit the instance part, but I don't think that is guaranteed. The ASCII sanitization is indeed missing (even in the current implementation).

Really the easiest way is just to follow the spec and make the bus name org.mpris.MediaPlayer2.spotifyd.instance{pid} all of the time and not even use the DeviceName at all.

I can imagine that some use cases might benefit from the explicit naming with DeviceName, since it makes the bus names reproducible between restarts and can be hard coded in a custom client that targets specifically one instance. I personally don't have a use case for this flag, so I don't know in which situations people would be using it. Might as well be that the pid version is just sufficient for those who need it. Probably something that @klay2000 can answer best.

[JasonLG1979]

I don't know in which situations people would be using it. Might as well be that the pid version is just sufficient for those who need it. Probably something that @klay2000 can answer best.

I'm not sure either? If it's just for the sake of uniqueness purely to allow for multiple instances I would follow the specs suggestion, and really that could just be the default behavior all of the time.

On the other hand if it's to let a user know what the instance is in any sort of UI you'd want to set the Identity prop to something like Spotifyd {DeviceName} Identity can be any arbitrary string, it's not bound by the bus name rules, and that's what would be displayed to a user not the bus name. You may already do this idk?

[klay2000]

Reading the spec, it actually only says that it must have a dot and a unique identifier appended to it's bus name, the instance{pid} thing is just an example so I think something like org.mpris.MediaPlayer2.spotifyd.{DeviceName} is sufficient to conform to the spec.

As far as ASCII sanitization goes I definitely forgot about that one, I'll make these changes and push it up.

[JasonLG1979]

Do you need it to be the device name for any reason in particular? If it's just for the sake of having a unique name I strongly suggest using what the example suggests as it is the convention that all other players follow. It's also useful to clients because if it's the pid it can be used for other things.

[klay2000]

I suppose either is useful but I've been debugging something that uses this feature and I've been running into issues with Spotifyd's PID not being the same as the one it has on creation when I try to kill it's process, so at the moment it seems like it's not going to be as easy to make use of.

[JasonLG1979]

Pids don't change over the course of a program's life. You're prob getting a pid of a child process/thread of Spotifyd mixed up with the main process/thread.

[JasonLG1979]

Not to mention that 2 equally valid device names could be sanitized into the same name thus breaking the uniqueness guarantee.

[klay2000]

Exactly, the parent process is dying and leaving several child processes.

[klay2000]

Alright, I'll change it to the PID.

[JasonLG1979]

My guess would be that you want the pid of the Tokio runtime.

[JasonLG1979]

If you try to grab a pid before you're in the runtime it may be wrong?

[JasonLG1979]

I suppose either is useful but I've been debugging something that uses this feature and I've been running into issues with Spotifyd's PID not being the same as the one it has on creation when I try to kill it's process, so at the moment it seems like it's not going to be as easy to make use of.

I sprinkled some println!("PID in 'whatever': {}", std::process::id()); all around the current dev version of librespot and they all printed the same PID.

My guess is that if you're using systemd (or some other way to daemonize Spotifyd) is that you're not getting the actual PID but the PID of the task that spawns Spotifyd?

[JasonLG1979]

This whole PR could be as simple as:

    let busname = format!(
        "org.mpris.MediaPlayer2.spotifyd.instance{}",
        std::process::id()
    );
    
    conn.request_name(&busname, true, true, true)
        .await
        .expect("Failed to register dbus player name");

[JasonLG1979]

You don't need all the b = stuff, you've only got one value and format args can be positional. The only reason to assign them is if it needs to be repeated in the string.

[eladyn]

LGTM, thanks for your work!