Repository: Spotifyd/spotifyd (language: rust)

Issue:
MPRIS Seek and SetPosition implementations are incompatible with what playerctl expects
**Description**

Basic playback control with `playerctl` is possible, but triggers an error or segfault when attempting to seek to an absolute/relative position. When inspecting the D-Bus interfaces with `d-feet`, it becomes obvious that `spotifyd` offers a different interface from all other software implementing the MPRIS specification.

![20220907_132647](https://user-images.githubusercontent.com/3588994/188867393-ebce5810-a1d3-41a5-ad69-57f23c7309e2.png)

The [Seek](https://specifications.freedesktop.org/mpris-spec/latest/Player_Interface.html#Method:Seek) method expects a signed int64 to be able to seek backwards and forwards. Additionally, [it is currently interpreted in milliseconds](https://github.com/Spotifyd/spotifyd/blob/7146f64a819590a16de9991de74f8bc2fea9c2ac/src/dbus_mpris.rs#L268), but should be interpreted in microseconds.

The [SetPosition](https://specifications.freedesktop.org/mpris-spec/latest/Player_Interface.html#Method:SetPosition) method expects a string trackid (the rationale being "to avoid race conditions where a client tries to seek to a position when the track has already changed") and a signed int64 in microseconds. Given that the implementation looks very similar to the Seek one, it's likely it's interpreted in milliseconds instead.

**To Reproduce**

- `playerctl -p spotifyd position 10` to set absolute position to 10s
- `playerctl -p spotifyd position 10+` to seek forward by 10s
- `playerctl -p spotifyd position 10-` to seek backward by 10s

To use d-feet:

- `d-feet`
- Click on "Session Bus"
- Narrow down search results by entering "mpris"
- Select the spotifyd one
- Click on the `/org/mpris/MediaPlayer2` path
- Click on the `org.mpris.MediaPlayer2.Player` interface
- Click on the methods

**Expected behavior**

- spotifyd playback continues at 10s or 10s later or 10s earlier

**Actual behavior**

The first playerctl call segfaults:

    (playerctl:251381): GLib-CRITICAL **: 13:52:57.116: g_variant_new_object_path: assertion 'g_variant_is_object_path (object_path)' failed

The second and third fail with a D-Bus error:

    Could not execute command: GDBus.Error:org.freedesktop.DBus.Error.Failed: D-Bus argument type mismatch at position 0: expected u32, found i64

**Logs**
<details><summary>Click to show logs</summary>

No logs, duh

</details>

**Compilation flags** 
- [ ] dbus_mpris
- [ ] dbus_keyring
- [x] alsa_backend
- [ ] portaudio_backend
- [ ] pulseaudio_backend
- [ ] rodio_backend

**Versions (please complete the following information):**

- OS: Arch Linux
- Spotifyd: 0.3.3 (7146f64a819590a16de9991de74f8bc2fea9c2ac) https://aur.archlinux.org/packages/spotifyd-full-git
- cargo: 1.63.0


Discussion / hints:
Thanks for the report! Apart from the two that you discovered, there are also some other methods that don't strictly conform to the spec. (There is only `VolumeUp` and `VolumeDown`, but no `Volume`.)

But the one that you mention should be quite easy to fix, I'll create a PR.

Fix patch:
diff --git a/src/dbus_mpris.rs b/src/dbus_mpris.rs
index 66a8a68d..c2ae0aaf 100644
--- a/src/dbus_mpris.rs
+++ b/src/dbus_mpris.rs
@@ -29,7 +29,7 @@ use rspotify::{
     prelude::*,
     AuthCodeSpotify, Token as RspotifyToken,
 };
-use std::{collections::HashMap, env, pin::Pin, sync::Arc};
+use std::{collections::HashMap, convert::TryInto, env, pin::Pin, sync::Arc};
 use tokio::sync::mpsc::{UnboundedReceiver, UnboundedSender};
 
 pub struct DbusServer {
@@ -261,13 +261,33 @@ async fn create_dbus_server(
 
         let mv_device_name = device_name.clone();
         let sp_client = Arc::clone(&spotify_api_client);
-        b.method("Seek", ("pos",), (), move |_, _, (pos,): (u32,)| {
+        b.method("Seek", ("offset",), (), move |_, _, (pos,): (i64,)| {
             if let Ok(Some(playback)) = sp_client.current_playback(None, None::<Vec<_>>) {
                 if playback.device.name == mv_device_name {
-                    let _ = sp_client.seek_track(
-                        playback.progress.map(|d| d.as_millis()).unwrap_or(0) as u32 + pos,
-                        playback.device.id.as_deref(),
-                    );
+                    let new_pos = playback
+                        .progress
+                        .and_then(|d| d.as_millis().try_into().ok())
+                        .and_then(|d: i64| d.checked_add(pos / 1000));
+
+                    if let Some(new_pos) = new_pos {
+                        let duration: u32 = match playback.item {
+                            Some(PlayableItem::Track(t)) => t.duration.as_millis(),
+                            Some(PlayableItem::Episode(e)) => e.duration.as_millis(),
+                            None => return Ok(()),
+                        }
+                        .try_into()
+                        .unwrap_or(u32::MAX);
+
+                        // MPRIS spec: negative values should be treated as 0
+                        let new_pos = new_pos.max(0);
+                        if new_pos <= duration as i64 {
+                            let _ =
+                                sp_client.seek_track(new_pos as u32, playback.device.id.as_deref());
+                        } else {
+                            // MPRIS spec: values beyond track bounds should act like Next
+                            let _ = sp_client.next_track(playback.device.id.as_deref());
+                        }
+                    }
                 }
             }
             Ok(())
@@ -275,14 +295,41 @@ async fn create_dbus_server(
 
         let mv_device_name = device_name.clone();
         let sp_client = Arc::clone(&spotify_api_client);
-        b.method("SetPosition", ("pos",), (), move |_, _, (pos,): (u32,)| {
-            if let Ok(Some(playback)) = sp_client.current_playback(None, None::<Vec<_>>) {
-                if playback.device.name == mv_device_name {
-                    let _ = sp_client.seek_track(pos, playback.device.id.as_deref());
+        b.method(
+            "SetPosition",
+            ("track_id", "position"),
+            (),
+            move |_, _, (track_id, pos): (dbus::Path, i64)| {
+                if let Ok(Some(playback)) = sp_client.current_playback(None, None::<Vec<_>>) {
+                    let (track_matches, duration) = if let Some(item) = playback.item {
+                        let track_matches = item
+                            .id()
+                            .map(|id| uri_to_object_path(id.uri()) == track_id)
+                            .unwrap_or(false);
+                        let duration = match item {
+                            PlayableItem::Track(t) => t.duration.as_micros(),
+                            PlayableItem::Episode(e) => e.duration.as_micros(),
+                        }
+                        .try_into()
+                        .unwrap_or(i64::MAX);
+                        (track_matches, duration)
+                    } else {
+                        return Ok(());
+                    };
+
+                    // as per MPRIS spec: pos must be in track bounds and track id must match
+                    if playback.device.name == mv_device_name
+                        && track_matches
+                        && (0..=duration).contains(&pos)
+                    {
+                        // pos is in microseconds, seek_track takes milliseconds
+                        let _ = sp_client
+                            .seek_track((pos / 1000) as u32, playback.device.id.as_deref());
+                    }
                 }
-            }
-            Ok(())
-        });
+                Ok(())
+            },
+        );
 
         let mv_device_name = device_name.clone();
         let sp_client = Arc::clone(&spotify_api_client);
@@ -462,7 +509,7 @@ async fn create_dbus_server(
                     .current_playback(None, None::<Vec<_>>)
                     .ok()
                     .flatten()
-                    .and_then(|p| Some(p.progress?.as_nanos() as i64))
+                    .and_then(|p| Some(p.progress?.as_micros() as i64))
                     .unwrap_or(0);
 
                 Ok(pos)
@@ -526,7 +573,7 @@ async fn create_dbus_server(
             .recv()
             .await
             .expect("Changed track channel was unexpectedly closed");
-        let mut seeked_position = None;
+        let mut seeked_position_ms = None;
 
         // Update playback state from event
         let (track_id, playback_status, player_volume) = match event {
@@ -538,7 +585,7 @@ async fn create_dbus_server(
                 position_ms,
                 ..
             } => {
-                seeked_position = Some(position_ms);
+                seeked_position_ms = Some(position_ms);
                 (Some(track_id), Some(PlaybackStatus::Playing), last_volume)
             }
             PlayerEvent::Stopped { .. } => {
@@ -625,18 +672,28 @@ async fn create_dbus_server(
         }
 
         // if position in track has changed emit a Seeked signal
-        if let Some(position) = seeked_position {
+        if let Some(position_ms) = seeked_position_ms {
             let msg = dbus::message::Message::signal(
                 &dbus::Path::new("/org/mpris/MediaPlayer2").unwrap(),
                 &dbus::strings::Interface::new("org.mpris.MediaPlayer2.Player").unwrap(),
                 &dbus::strings::Member::new("Seeked").unwrap(),
             )
-            .append1(position as i64);
+            // position should be in microseconds
+            .append1(position_ms as i64 * 1000);
             conn.send(msg).unwrap();
         }
     }
 }
 
+fn uri_to_object_path(uri: String) -> dbus::Path<'static> {
+    let mut path = String::with_capacity(uri.len() + 1);
+    for element in uri.split(':') {
+        path.push('/');
+        path.push_str(element);
+    }
+    dbus::Path::new(path).unwrap()
+}
+
 fn insert_metadata(m: &mut HashMap<String, Variant<Box<dyn RefArg>>>, item: PlayableItem) {
     use rspotify::model::{
         Image,
@@ -653,7 +710,7 @@ fn insert_metadata(m: &mut HashMap<String, Variant<Box<dyn RefArg>>>, item: Play
 
     // a common denominator struct for FullEpisode and FullTrack
     struct TrackOrEpisode {
-        id: Option<String>,
+        id: Option<dbus::Path<'static>>,
         duration: std::time::Duration,
         images: Vec<Image>,
         name: String,
@@ -665,7 +722,7 @@ fn insert_metadata(m: &mut HashMap<String, Variant<Box<dyn RefArg>>>, item: Play
 
     let item = match item {
         Track(t) => TrackOrEpisode {
-            id: t.id.map(|t| t.uri()),
+            id: t.id.map(|t| uri_to_object_path(t.uri())),
             duration: t.duration,
             images: t.album.images,
             name: t.name,
@@ -680,7 +737,7 @@ fn insert_metadata(m: &mut HashMap<String, Variant<Box<dyn RefArg>>>, item: Play
             }),
         },
         Episode(e) => TrackOrEpisode {
-            id: Some(e.id.uri()),
+            id: Some(uri_to_object_path(e.id.uri())),
             duration: e.duration,
             images: e.show.images,
             name: e.name,


Test patch: