SpircHandler: only advance the queue for the entry that actually started - #2
Open
007hacky007 wants to merge 4 commits into
Open
SpircHandler: only advance the queue for the entry that actually started#2007hacky007 wants to merge 4 commits into
007hacky007 wants to merge 4 commits into
Conversation
The track queue is accessed from three threads: the mercury/session thread (handleFrame -> updateTracks/skipTrack), the TrackPlayer task (consumeTrack) and the queue task (runTask/processTrack), plus whatever thread the integration notifies track starts from. tracksMutex is meant to serialize them, but several paths bypassed it: - runTask() read currentTracksIndex outside the mutex - processTrack() called queueNextTrack(), which mutates preloadedTracks, from the queue task without holding the mutex - consumeTrack() indexed preloadedTracks[0] without an empty check, and updateTracks() dereferenced preloadedTracks[0] the same way Bring those reads and mutations under tracksMutex and guard the two empty-deque dereferences. Groundwork for making notification handling a single locked queue transaction.
notifyAudioReachedPlayback() advanced the track queue blindly: it either consumed the notifyPending flag or popped the queue head, without checking which track the notification was actually for. A notification from a stream created before the last queue rebuild (it races the player reload that follows a Load frame, e.g. when another Connect controller changes tracks) consumed the notifyPending meant for the new head. From then on every legitimate track start popped the queue one entry early, TrackPlayer's previous-track pointer was never found in preloadedTracks again, and every EOF restarted playback from position 0 via trackLoaded(), cutting off everything still buffered in the renderer. There is no resync path, so playback stayed corrupted (every song cut short, UI one track ahead of the audio) until the process was restarted. Two ingredients make the validation sound: - Queue entries get an identity, not just a file id: a process-unique sequence number is appended to the identifier at construction. A file id alone cannot prove which playback instance started (same track queued twice in a row, PREV restarting the current track as a new object), and as a bonus integrations that detect track boundaries by identifier change now see one for consecutive plays of the same file. - Validation and advancement are one atomic queue transaction: TrackQueue::notifyTrackReached() checks the identifier against the head (pending-notify case) or the head's successor (advance case) and performs the pop under a single tracksMutex hold, so a Load/Replace frame rebuilding the queue from the mercury thread cannot interleave between check and pop. Callers that cannot identify the playing track (flow mode, spotraop, the CLI player) pass an empty identifier and keep the legacy behavior; the parameter is a std::string_view with an empty default, so untouched call sites neither change behavior nor allocate.
Two more paths read queue state without tracksMutex: - getTrackInfo() iterated preloadedTracks while the mercury thread can clear and repopulate the deque in updateTracks(); concurrent iteration and mutation of the deque is undefined behavior. The per-entry identifiers make this path hotter, since consecutive plays of the same file now trigger the integrations' track handler. - the nanopb encode callback registered for innerFrame copied currentTracks whenever a frame was encoded (any notify(), from any thread) while updateTracks() can assign a new vector concurrently. The callback argument now carries the list together with its guard and the encoder holds it for the duration of the memory-only encode. Frame sends happen strictly after queue calls return, never under tracksMutex, so no lock-order cycle is introduced.
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 with spotupnp and two Connect controllers (playback started from one device, tracks changed from the other): after a burst of track changes, every following song was cut short by 60-90 seconds and the Spotify UI showed a track one ahead of what was actually playing. Only a restart recovered it.
What happens
notifyAudioReachedPlayback()advances the queue blindly: it either consumesnotifyPendingor pops the head, without checking which track the notification is for.A track change from a second controller arrives as a full
Loadframe. The handler rebuilds the queue immediately, but the player reload (and thePLAYBACK_STARTthat clears the integration's streamers) happens on the TrackPlayer task a few hundred ms later. A "reached playback" notification from a stream created before the rebuild can land in that window and consume thenotifyPendingthat belongs to the new head. From then on the accounting is off by one for good:TrackPlayer's previous-track pointer is never found inpreloadedTracksagain, so every data-feed EOF goes throughtrackLoaded()and restarts playback from scratch instead of feeding the next track in the backgroundThere is no resync path.
Fix
QueuedTrackgets a process-unique sequence appended to its identifier (<fileIdHex>#<seq>). A file id alone cannot prove which playback instance started: the same track twice in a row, or PREV re-queueing the current track, are indistinguishable. Side benefit: integrations that detect track boundaries by identifier change now get one between consecutive plays of the same file, which previously merged into one stream.TrackQueue::notifyTrackReached()validates the identifier against the head (pending case) or the head's successor (advance case) and pops under a singletracksMutexhold, so a Load/Replace rebuilding the queue on the mercury thread cannot interleave between check and pop. Stale or duplicate notifications are logged and ignored.tracksMutex(unlockedcurrentTracksIndexread inrunTask(),queueNextTrack()from the queue task, unlockedgetTrackInfo()iteration, the nanopb encode callback copyingcurrentTracksmid-rebuild) plus two empty-deque dereferences - without those the transaction would not be reliable.The parameter is a
std::string_viewdefaulting to empty, which keeps the exact legacy behavior for callers that cannot identify the playing track (flow mode, spotraop, the CLI player) - those compile and behave unchanged.Notes
Reproduced against a Hama DIT2010: with rapid track changes from a second controller the log shows the stale starts being rejected (
stale notification for <id>#10 while expecting <id>#13 => ignored) and playback staying in sync; normal gapless transitions validate the successor and advance exactly once; full tracks play to the end.This change is safe on its own for all existing callers; a small spotupnp PR follows that passes the started streamer's
trackUniqueat the UPnP call site.