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

p.trackLengthMs is never reset on load, so a track with no database row inherits the previous track's duration #125

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

p.trackLengthMs is the authoritative duration the position display is scaled against. It is written in exactly one place — getCurrentTrackInfoLocked, and only when the database lookup succeeds (backend/player/player.go:1088-1095):

meta, err := p.db.ReadQueries.GetTrackByPath(p.ctx, info.FilePath)
if err == nil {
	...
	p.trackLengthMs = meta.LengthMilliseconds

and zeroed in exactly one place, UnloadTrack (:767). Loading a track does not reset it. So when the new track has no row — a file played from outside the library, a path the scan has not reached, a row removed by "Remove from library" — the previous track's duration is silently retained.

That value is not cosmetic. displayPositionSecsLocked (:1154) reports pos / total * displayLength, so a stale displayLength rescales every position report; trackLengthLocked returns it as the track's length, so the seek bar's max is the old track's too, and seekLocked converts a target back through the same ratio. A short track following a long one reports positions that crawl and a bar that never fills; the reverse races to the end and stops.

Direction

Reset p.trackLengthMs = 0 in loadFileLocked before emitTrackChanged(), so a missing row falls through to the decoder-derived length (:1131) instead of inheriting. Worth doing together with the p.format fix, since that fallback is the other half of the same expression.

**Findings** `p.trackLengthMs` is the authoritative duration the position display is scaled against. It is written in exactly one place — `getCurrentTrackInfoLocked`, and only when the database lookup **succeeds** (`backend/player/player.go:1088-1095`): ```go meta, err := p.db.ReadQueries.GetTrackByPath(p.ctx, info.FilePath) if err == nil { ... p.trackLengthMs = meta.LengthMilliseconds ``` and zeroed in exactly one place, `UnloadTrack` (`:767`). Loading a track does not reset it. So when the new track has no row — a file played from outside the library, a path the scan has not reached, a row removed by "Remove from library" — **the previous track's duration is silently retained**. That value is not cosmetic. `displayPositionSecsLocked` (`:1154`) reports `pos / total * displayLength`, so a stale `displayLength` rescales every position report; `trackLengthLocked` returns it as the track's length, so the seek bar's `max` is the old track's too, and `seekLocked` converts a target back through the same ratio. A short track following a long one reports positions that crawl and a bar that never fills; the reverse races to the end and stops. **Direction** Reset `p.trackLengthMs = 0` in `loadFileLocked` before `emitTrackChanged()`, so a missing row falls through to the decoder-derived length (`:1131`) instead of inheriting. Worth doing together with the `p.format` fix, since that fallback is the other half of the same expression.
yonlu self-assigned this 2026-08-19 12:39:08 +00:00
yonlu added the
Status
In Progress
 label 2026-08-19 12:39:09 +00:00
yonlu commented 2026-08-19 12:39:09 +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:58 +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
Low
4
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#125