spotupnp: do not mistake our own stop acknowledgment for a user stop - #68
Open
007hacky007 wants to merge 1 commit into
Open
spotupnp: do not mistake our own stop acknowledgment for a user stop#68007hacky007 wants to merge 1 commit into
007hacky007 wants to merge 1 commit into
Conversation
shadowRequest(SPOT_STOP) arms ExpectStop when sending AVTStop, but SPOT_PLAY cleared it as soon as the follow-up play command was sent. A track change (spirc Load frame) issues Stop, SetURI and Play within about 100 ms; when the renderer is slow to execute them (typically when the change happens while paused), the next transport-state poll still returns STOPPED after the play was sent. With ExpectStop already cleared, ActionHandler treats that stale STOPPED as a user-initiated stop and sends SHADOW_STOP, which aborts the Spotify session (disconnect on unexpected STOP) - the device drops from the user's account and they have to reconnect and press play. Depending on playlistEnd it can instead reset the new track to paused. Keep ExpectStop armed until ActionHandler actually observes a transport state: an expected STOPPED is ignored and consumes the flag, and TRANSITIONING, PLAYING or PAUSED_PLAYBACK observations disarm it. Because those observations only fire on a state *change*, a renderer that coalesces states could leave the flag armed indefinitely, which would swallow a later genuine user stop. The suppression therefore only honors the flag within 5 seconds of the stop command (ExpectStopTime); past the window a STOPPED is treated as genuine again, bounding the worst case instead of trading one permanent failure for another. A suppressed expected stop also skips the branch that would have freed a pending gapped-next URL, so a stale NextStreamUrl could survive a track change and divert the end of the newly loaded track. SPOT_LOAD now clears NextStreamUrl whenever it performs a full SetTrackURI load, which makes that state impossible regardless of suppression timing. Reproduced deterministically against a Hama DIT2010 by changing track while paused; with this change the late STOPPED ack is consumed silently and the session survives.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hit this on my setup: changing track while paused kicked the renderer off my Spotify account - I had to re-select the device and press play. Reproduced at will with pause, wait a few seconds, change track.
What happens
A track change (spirc
Loadframe) restarts the renderer with Stop, SetURI and Play within ~100 ms.SPOT_STOParmsExpectStopso the STOPPED that acknowledges our ownAVTStopis not mistaken for a user stop - butSPOT_PLAYclears the flag as soon as the play command is sent, not when the renderer has been observed past the stop. When the renderer is slow to execute (typical when it was paused), the next transport-state poll still returns STOPPED after the play:ActionHandler treats the stale STOPPED as user-initiated and the "disconnect on unexpected STOP" logic releases the player. It usually goes unnoticed because the poll tends to land inside the short
SpotState == SPOT_STOPwindow, or the renderer transitions fast enough that STOPPED is never polled.Fix
ExpectStoparmed until ActionHandler actually observes a transport state: an expected STOPPED is consumed silently, and TRANSITIONING / PLAYING / PAUSED_PLAYBACK observations disarm itExpectStopTime). Past the window a STOPPED is genuine again - worst case is one absorbed stop right after a track change, instead of either permanent failureSPOT_LOADnow frees any pendingNextStreamUrlwhen it does a fullSetTrackURIload; a suppressed expected stop skips the branch that used to free it, and a stale gapped-next URL must not survive into the new trackNotes
With the fix, the same repro shows the late acknowledgment being absorbed (
AVTStop.203,AVTPlay.284, STOPPED polled .604 - no disconnect) and playback continues; a genuine renderer stop still frees the session as designed. Independent of the stale-notification PR.