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

p.format is never updated on load: replaying a finished 48 kHz track resamples 44100 to 44100 #124

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

Findings

Player.format is assigned exactly once, in the constructor, to the speaker's sample rate (backend/player/player.go:151-155):

state:        Stopped,
baseStreamer: generators.Silence(-1),
format: beep.Format{
	SampleRate: speakerSampleRate,
},

loadFileLocked decodes the file and gets a real beep.Format back from metadata.DecodeFile, then passes only format.SampleRate to updateStreamers and drops the rest on the floor — p.format is never updated. So p.format.SampleRate is 44100 for the life of the process regardless of what is playing. Three call sites read it:

  • player.go:658 — the replay path, and this one is audible. When a track has finished naturally and is played again, Play() rebuilds the chain with p.updateStreamers(p.seeker, p.format.SampleRate), i.e. it declares the decoder's output to be 44100 Hz. For a 48 kHz file the first load resampled 48000→44100 correctly (the rate came from the decoder), and the replay resamples 44100→44100 — a no-op. The track plays back at the wrong speed and pitch, and its position/length arithmetic goes with it. Press play on a finished 48 kHz track and you hear it.
  • player.go:1144 — trackLengthLocked's fallback, p.seeker.Len() / int(p.format.SampleRate), used whenever the database has no duration for the track (an unscanned or unmatched file). Wrong duration for anything not 44.1 kHz.
  • player.go:1170 — displayPositionSecsLocked's fallback, same division, same condition.

Direction

Assign p.format = format in loadFileLocked alongside p.currentFile = f, and take the replay path's rate from it. A test that loads a 48 kHz fixture and asserts trackLengthLocked against the known duration covers all three, and internal/testfixtures can select by case.

**Findings** `Player.format` is assigned exactly once, in the constructor, to the *speaker's* sample rate (`backend/player/player.go:151-155`): ```go state: Stopped, baseStreamer: generators.Silence(-1), format: beep.Format{ SampleRate: speakerSampleRate, }, ``` `loadFileLocked` decodes the file and gets a real `beep.Format` back from `metadata.DecodeFile`, then passes only `format.SampleRate` to `updateStreamers` and **drops the rest on the floor** — `p.format` is never updated. So `p.format.SampleRate` is 44100 for the life of the process regardless of what is playing. Three call sites read it: - **`player.go:658` — the replay path, and this one is audible.** When a track has finished naturally and is played again, `Play()` rebuilds the chain with `p.updateStreamers(p.seeker, p.format.SampleRate)`, i.e. it declares the decoder's output to be 44100 Hz. For a 48 kHz file the first load resampled 48000→44100 correctly (the rate came from the decoder), and the replay resamples 44100→44100 — a no-op. **The track plays back at the wrong speed and pitch**, and its position/length arithmetic goes with it. Press play on a finished 48 kHz track and you hear it. - **`player.go:1144`** — `trackLengthLocked`'s fallback, `p.seeker.Len() / int(p.format.SampleRate)`, used whenever the database has no duration for the track (an unscanned or unmatched file). Wrong duration for anything not 44.1 kHz. - **`player.go:1170`** — `displayPositionSecsLocked`'s fallback, same division, same condition. **Direction** Assign `p.format = format` in `loadFileLocked` alongside `p.currentFile = f`, and take the replay path's rate from it. A test that loads a 48 kHz fixture and asserts `trackLengthLocked` against the known duration covers all three, and `internal/testfixtures` can select by case.
yonlu self-assigned this 2026-08-19 12:39:07 +00:00
yonlu added the
Status
In Progress
 label 2026-08-19 12:39:07 +00:00
yonlu commented 2026-08-19 12:39:07 +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:50 +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
Medium
3
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#124