Sinono3 / souvlaki

A cross-platform library for handling OS media controls and metadata.
MIT License
80 stars 14 forks source link

`dbus-crossroads` backend does not immediately apply updates #48

Open Beinsezii opened 6 months ago

Beinsezii commented 6 months ago

The best way I can describe this is that frequent updates to the state will get queued but not actually applied until it's read

So for example, if you run an application containing something like

loop {
  controls.set_playback(MediaPlayback::Playing {progress: position});
  thread::sleep(Duration::from_secs(1));
}

for 10 seconds, then call watch -t 1 playerctl position, you'd expect it to look like 11.0 12.0 13.0 but what you actually get is 1.0 2.0 3.0

And if you shorten the watch duration to a smaller poll rate watch -t 0.5 playerctl position, you'd expect 11.0 11.0 12.0 12.0 because the app updates every second, but in reality it's 1.0 2.0 3.0 4.0 again, as it's working through a bunch of 'queued' updates I guess.

As a bonus kicker, the timeout is either extremely long or nonexistent as I can call playerctl position minutes after playback has stopped and it'll still have dozens or hundreds of updates 'queued' before I can read its true state.

The use_zbus backend does not demonstrate this issue, and shows accurate states even with hundreds of metadata updates a second.

I have basically no knowledge of how D-Bus operates, so if I can provide more information let me know.

Sinono3 commented 5 months ago

Okay. I think this is because we use a MPSC channel internally (std::sync::mpsc) and we check for these messages using if let instead of while let. This causes us to only get one message every 500 ms which is very bad. We can fix this in two ways:

1) The quick way is changing that if let for a while let, which I think should work. If it blocks the thread for too long we might have to find a way to make it non-blocking. 2) Switching our thread-shared state management to use Arc<Mutex<T>> or Arc<RwLock<T>>. The switch shouldn't be difficult.

Can you try doing one of this fixes on a local copy of souvlaki and see if it works?

Beinsezii commented 5 months ago

Why is conn.process() set to 1000ms by default? In fact why does it even need a duration?

Setting conn.process(Duration::from_millis(0))?; lets me read playback position in real-time like on zbus and so far there's no adverse side effects.

Beinsezii commented 5 months ago

I also don't get the whole loop{if let} vs while let thing. Shouldn't they be functionally equivalent? Where's the 500ms come from?

Beinsezii commented 5 months ago

Okay I see what you meant with while let.

diff --git a/src/platform/mpris/dbus/controls.rs b/src/platform/mpris/dbus/controls.rs
index 6aa5123..a59e228 100644
--- a/src/platform/mpris/dbus/controls.rs
+++ b/src/platform/mpris/dbus/controls.rs
@@ -242,10 +242,10 @@ where
         }),
     );

-    loop {
-        if let Ok(event) = event_channel.recv_timeout(Duration::from_millis(10)) {
+    'outer: loop {
+        while let Ok(event) = event_channel.recv_timeout(Duration::from_millis(10)) {
             if event == InternalEvent::Kill {
-                break;
+                break 'outer;
             }

             let mut changed_properties = HashMap::new();

To consume all the events at once before processing. That also works. I still find the 1000ms wild though.