Time position isn't zeroed immediately when changing track

[jodal]

Currently, time position is zeroed a tiny bit after a track change happens. This can be seen in this MPD dialog:

# TLID 4 playing, time position correct
status
volume: 78
repeat: 0
random: 0
single: 0
consume: 0
playlist: 11
playlistlength: 11
xfade: 0
state: play
song: 4
songid: 4
time: 235:237
elapsed: 235.682
bitrate: 320
OK

# TLID 5 playing, time position from TLID 4
status
volume: 78
repeat: 0
random: 0
single: 0
consume: 0
playlist: 11
playlistlength: 11
xfade: 0
state: play
song: 5
songid: 5
time: 237:219
elapsed: 237.669
bitrate: 320
OK

# TLID 5 playing, time position correct
status
volume: 78
repeat: 0
random: 0
single: 0
consume: 0
playlist: 11
playlistlength: 11
xfade: 0
state: play
song: 5
songid: 5
time: 1:219
elapsed: 1.811
bitrate: 320
OK

Some clients, like ncmpcpp, doesn't query status regularly, but calculate the displayed time position on the client side. Thus, a single wrong time position like seen above is enough to make the client show the wrong time position throughout the track. This also makes seeking hard, since you don't really know where in the track you currently are.

[adamcik]

I believe this is a result of the get_state() calls we removed to get spotify seeks / time tracking working.

[jodal]

Yeah, that would make sense. We no longer block for any pending state transitions to complete before returning from get_position().

Any ideas for hacks/workarounds that could work?

[jodal]

I'm not sure how we can fix this, except for reintroducing the blocking get_state() calls I'm sure we removed for a reason. Suggestions?

[adamcik]

Correct fix is to re-add the blocking code.

[adamcik]

I have some thoughts around this as part of the audio cleanups. Idea is basically to have async actions like seek return threading events that we can wait for in the actor making the change.

[adamcik]

New plan is that we should update the audio actor to store the seek target, then instead of waiting for the new segment, we wait for async done at which point we can emit the seek target. This implies that any other actions that would trigger async done must always null out the seek target. And from cores point of view it would still just wait for the position changed event.

[adamcik]

Double checked this now. Having the audio event come from the new segment seems just fine, moving to async done is not needed. But either way we need to know if a seek is active to effectively avoid the initial segment, or async dones due to other state changes.

It should still be noted that though the better event triggering is nice, it's more a question of #1331 and we still have the issue of position queries failing between the seek being initiated and completing.

[adamcik]

Just having a wait_for_state_change() in the get_position() call might also be enough, but I'm slightly worried about that with respect to deadlocking.

[adamcik]

Separate seek lock that gets held while a seek is underway, and reset by all and any async done might be safer. But haven't thought through enough of the consequences just yet.