yonlu/yellowjacket
1
1
Fork 0
Code Issues 55 Pull Requests 6 Actions Packages Projects Releases 12 Wiki Activity

The player can sit in Playing forever with the position frozen: BufferedStreamer streams silence with no end condition #122

New Issue
Closed
opened 2026-08-19 12:37:37 +00:00 by yonlu · 1 comment
yonlu commented 2026-08-19 12:37:37 +00:00
Owner
Copy Link
Copy Source

Report

Observed once, not reproduced: the play/pause button showed pause — implying playback — while the seek bar sat still.

Findings

BufferedStreamer.Stream treats an empty ring buffer as a temporary underrun and returns silence forever (backend/player/buffered_streamer.go:156):

if bs.count == 0 {
	// Buffer temporarily empty — fill with silence.
	for i := range samples {
		samples[i] = [2]float64{}
	}

	return len(samples), true
}

That is only correct while the read-ahead goroutine is still going to deliver something. readAhead has two exit paths that do not set done (:57-135): <-bs.closed, and a source that keeps returning (0, true) — it sleeps 1 ms and loops forever. After either, the ring drains to zero and Stream returns (len(samples), true) for the rest of the process's life.

Everything downstream then reads as healthy playback:

  • the beep.Seq chain never returns 0, false, so the playback-finished callback never fires, so p.state stays Playing — the button keeps showing pause, IsPlaying() keeps saying yes, and auto-advance never happens;
  • p.seeker.Position() never advances, so emitPositionIfPlaying (player.go:261-291) keeps emitting the same positionSeconds once a second with a fresh seq. The seek bar applies each report and calls stopProgress() (seek-bar.ts updated()), so the backend report actively suppresses the interpolation that would otherwise have moved the bar. Frozen bar, pause icon, silence.

Nothing detects it. There is no watchdog comparing "state is Playing" against "the position moved", and the silence fill is unbounded.

Second defect in the same file: bs.Err() is read by nobody. grep -n 'Err()' backend/player/player.go returns nothing. When the source does fail, readAhead sets done and bs.err (:117-125), Stream returns 0, false, and the player runs onPlaybackFinished — so a mid-track decode error is indistinguishable from a track ending normally, and the queue cheerfully auto-advances. The user is told nothing; PlaybackFailed exists for exactly this and is not emitted.

Direction

  • Set done = true on every readAhead exit path, so a stopped producer ends the stream instead of stalling it.
  • Bound the underrun fill: after N consecutive silence-filled calls (a couple of seconds is far beyond any legitimate disk stall behind a 2 s read-ahead), return 0, false and record an error rather than pretending forever.
  • Surface bs.Err(): the finished path should distinguish "drained" from "failed" and emit PlaybackFailed for the latter, which is what makes the queue skip rather than silently advance.
  • A unit test per exit path — a source that returns (0, true) forever, and a Close() mid-stream — asserting Stream eventually reports end-of-stream.
**Report** Observed once, not reproduced: the play/pause button showed **pause** — implying playback — while the seek bar sat still. **Findings** `BufferedStreamer.Stream` treats an empty ring buffer as a temporary underrun and returns silence forever (`backend/player/buffered_streamer.go:156`): ```go if bs.count == 0 { // Buffer temporarily empty — fill with silence. for i := range samples { samples[i] = [2]float64{} } return len(samples), true } ``` That is only correct while the read-ahead goroutine is still going to deliver something. `readAhead` has **two exit paths that do not set `done`** (`:57-135`): `<-bs.closed`, and a source that keeps returning `(0, true)` — it sleeps 1 ms and loops forever. After either, the ring drains to zero and `Stream` returns `(len(samples), true)` for the rest of the process's life. Everything downstream then reads as healthy playback: - the `beep.Seq` chain never returns `0, false`, so the playback-finished callback never fires, so `p.state` stays `Playing` — **the button keeps showing pause**, `IsPlaying()` keeps saying yes, and auto-advance never happens; - `p.seeker.Position()` never advances, so `emitPositionIfPlaying` (`player.go:261-291`) keeps emitting the *same* `positionSeconds` once a second with a fresh `seq`. The seek bar applies each report and calls `stopProgress()` (`seek-bar.ts` `updated()`), so the backend report actively **suppresses** the interpolation that would otherwise have moved the bar. Frozen bar, pause icon, silence. Nothing detects it. There is no watchdog comparing "state is Playing" against "the position moved", and the silence fill is unbounded. Second defect in the same file: **`bs.Err()` is read by nobody.** `grep -n 'Err()' backend/player/player.go` returns nothing. When the source *does* fail, `readAhead` sets `done` and `bs.err` (`:117-125`), `Stream` returns `0, false`, and the player runs `onPlaybackFinished` — so a mid-track decode error is indistinguishable from a track ending normally, and the queue cheerfully auto-advances. The user is told nothing; `PlaybackFailed` exists for exactly this and is not emitted. **Direction** - Set `done = true` on every `readAhead` exit path, so a stopped producer ends the stream instead of stalling it. - Bound the underrun fill: after N consecutive silence-filled calls (a couple of seconds is far beyond any legitimate disk stall behind a 2 s read-ahead), return `0, false` and record an error rather than pretending forever. - Surface `bs.Err()`: the finished path should distinguish "drained" from "failed" and emit `PlaybackFailed` for the latter, which is what makes the queue skip rather than silently advance. - A unit test per exit path — a source that returns `(0, true)` forever, and a `Close()` mid-stream — asserting `Stream` eventually reports end-of-stream.
yonlu self-assigned this 2026-08-19 12:39:04 +00:00
yonlu added the
Status
In Progress
 label 2026-08-19 12:39:04 +00:00
yonlu commented 2026-08-19 12:39:05 +00:00
Author
Owner
Copy Link
Copy Source

Working this as part of a batch off fix/player-playing-state (#122, #123, #124, #125, #126) — all five are in backend/player and backend/queue, and #124 and #125 are the two halves of the same fallback expression, so splitting them across branches would mean three passes over the same twenty lines.

Approach:

  • #122 — done = true on every readAhead exit; bound the underrun fill and return 0, false past it; give the finished path a way to tell "drained" from "failed" so PlaybackFailed is emitted instead of a silent auto-advance.
  • #123 — move the two emits back under p.mu, and capture the chain's trackChangeID in the beep.Callback closure so a stale callback returns without touching the player.
  • #124/#125 — assign p.format in loadFileLocked, reset p.trackLengthMs there, take the replay path's rate from p.format.
  • #126 — bounds-check currentIndex the way loadCurrentTrack already does.

Verified with make test (all three build configurations) plus new unit tests per exit path; the audible half of #124 needs a 48 kHz fixture.

Working this as part of a batch off `fix/player-playing-state` (#122, #123, #124, #125, #126) — all five are in `backend/player` and `backend/queue`, and #124 and #125 are the two halves of the same fallback expression, so splitting them across branches would mean three passes over the same twenty lines. Approach: - **#122** — `done = true` on every `readAhead` exit; bound the underrun fill and return `0, false` past it; give the finished path a way to tell "drained" from "failed" so `PlaybackFailed` is emitted instead of a silent auto-advance. - **#123** — move the two emits back under `p.mu`, and capture the chain's `trackChangeID` in the `beep.Callback` closure so a stale callback returns without touching the player. - **#124/#125** — assign `p.format` in `loadFileLocked`, reset `p.trackLengthMs` there, take the replay path's rate from `p.format`. - **#126** — bounds-check `currentIndex` the way `loadCurrentTrack` already does. Verified with `make test` (all three build configurations) plus new unit tests per exit path; the audible half of #124 needs a 48 kHz fixture.
logan closed this issue 2026-08-19 14:53:46 +00:00
gitea-actions bot removed the
Status
In Progress
 label 2026-08-19 14:54:34 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Area/Player Kind/Bug Platform/Android Platform/Desktop
Priority
High
2
Reviewed
Confirmed
1
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees yonlu
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#122