Genre tags: read multi-value frames correctly, and repair existing rows #120

Merged

bvandeusen merged 2 commits from dev into main 2026-08-05 22:10:41 -04:00

Conversation 0 Commits 2 Files Changed 15

+1128 -49

bvandeusen commented 2026-08-05 22:10:31 -04:00

Owner

Copy Link

Copy Source

Fixes #2499. Two commits, server-only, carries migration 0054.

CI green on fd27819c (run 3456): sqlc verify, go vet, golangci-lint, go test (short, race), and the integration lane — which runs real migrations, so 0054 applied against Postgres.

What was wrong

internal/library/scanner.go stored dhowden/tag's meta.Genre() verbatim. That library's readTFrame does:

strings.Join(strings.Split(txt, string(singleZero)), "")

ID3v2 separates multiple values in a text frame with a null byte. That splits on the null and rejoins with the empty string, so a file tagged Alternative Rock + Rock was stored as the single token Alternative RockRock. It also leaves bare numeric ID3v1 references unresolved, which is why the library reported genres like 4017 and 526617.

Roughly two-thirds of the ~700 distinct "genres" in the operator's library were artifacts of this, not real labels.

Why it mattered beyond the browse axis

tracks.genre has four consumers and only the first is cosmetic:

• browse.sql — the genre index added in #367
• taste_profile.sql reads t.genre directly, so welded tokens were entering the taste profile's tag vocabulary, feeding TasteMatch and taste-tag overlap
• recommendation.sql / discover.sql split on [;,], which finds nothing to split — they were comparing welded pseudo-tags for genre similarity
• Subsonic getAlbumList?type=byGenre

Every count was also wrong: Rock reported 3413 while excluding every row welded into Alternative RockRock.

Why not ffprobe

ffprobe is already a dependency and was the obvious candidate, but ffmpeg's read_ttag calls decode_str once with no loop — it keeps only the first value and discards the rest. Trading welding for silent truncation of multi-genre tags would blunt the signal genre mainly feeds, so the frame is parsed directly instead.

internal/library/id3v2genre.go handles v2.2/2.3/2.4 frame layouts (2.4 synchsafe vs 2.3 plain sizes), all four text encodings, tag-level and v2.4 frame-level unsynchronisation, the extended header, and refuses compressed/encrypted frames. Everything other than the genre frame still comes from dhowden/tag.

Values are stored ;-delimited — a delimiter the read side already splits on — so no query changes and the blast radius stays in the scanner.

Repairing existing rows

The fix alone would have repaired nothing. scanFile's incremental skip short-circuits any file whose mtime hasn't moved, re-tagging doesn't move mtime, and no force-rescan affordance existed anywhere.

Migration 0054 adds tracks.tag_read_version (DEFAULT 0) against library.tagReadVersion = 1, following the precedent already in that skip clause for duration_ms = 0 backfill. Every existing row sorts below the constant and gets re-read on the next ordinary scan — no operator action, no rebuild. Bumping the constant is how a future extraction fix reaches an existing library, which is why it's a version rather than a needs_reread boolean.

A version-driven re-read reuses the stored duration instead of re-running ffprobe, since the file's bytes haven't moved. That keeps a library-wide repair pass tag-read-bound rather than one fork+exec per file.

taste_profile_tags needs nothing: BuildTasteProfile is an atomic delete-and-reinsert, so polluted rows are replaced on the next scheduled rebuild.

Scope

ID3v2 only. dhowden welds nowhere else — readTFrame is ID3v2-specific — so by construction every welded genre is an mp3. Confirmed independently by decoding the numerics against the ID3v1 table: 526617 = Electronic + New Wave + Rock, 1766Post-Punk43 = Rock + New Wave + Post-Punk + Punk. The Vorbis/MP4 repeated-field question is #2500, unproven and deliberately not built.

Tests

20 cases in internal/library/genre_test.go, including the Alternative RockRock regression across all three major versions, the operator's 8-value blob, both UTF-16 BOM placements, Latin-1 widening, and the unsynchronisation collapse (with a guard that fails if the test ever goes vacuous).

Also

Corrected two comments that misattributed this bug: recommendation/sessionvector.go blamed "broken tag-editor output" — it was ours — and api/library_browse.go now warns that #2468's taxonomy question must only be judged against a re-scanned library.

🤖 Generated with Claude Code

https://claude.ai/code/session_01N6vZoJ4Se5YyaqdtGVkap5

Fixes #2499. Two commits, server-only, **carries migration 0054**. CI green on `fd27819c` (run 3456): sqlc verify, `go vet`, golangci-lint, `go test (short, race)`, and the integration lane — which runs real migrations, so 0054 applied against Postgres. ## What was wrong `internal/library/scanner.go` stored `dhowden/tag`'s `meta.Genre()` verbatim. That library's `readTFrame` does: ```go strings.Join(strings.Split(txt, string(singleZero)), "") ``` ID3v2 separates multiple values in a text frame with a **null byte**. That splits on the null and rejoins with the **empty string**, so a file tagged `Alternative Rock` + `Rock` was stored as the single token `Alternative RockRock`. It also leaves bare numeric ID3v1 references unresolved, which is why the library reported genres like `4017` and `526617`. Roughly two-thirds of the ~700 distinct "genres" in the operator's library were artifacts of this, not real labels. ## Why it mattered beyond the browse axis `tracks.genre` has four consumers and only the first is cosmetic: - `browse.sql` — the genre index added in #367 - `taste_profile.sql` reads `t.genre` **directly**, so welded tokens were entering the taste profile's tag vocabulary, feeding `TasteMatch` and taste-tag overlap - `recommendation.sql` / `discover.sql` split on `[;,]`, which finds nothing to split — they were comparing welded pseudo-tags for genre similarity - Subsonic `getAlbumList?type=byGenre` Every count was also wrong: `Rock` reported 3413 while excluding every row welded into `Alternative RockRock`. ## Why not ffprobe ffprobe is already a dependency and was the obvious candidate, but ffmpeg's `read_ttag` calls `decode_str` **once** with no loop — it keeps only the first value and discards the rest. Trading welding for silent truncation of multi-genre tags would blunt the signal genre mainly feeds, so the frame is parsed directly instead. `internal/library/id3v2genre.go` handles v2.2/2.3/2.4 frame layouts (2.4 synchsafe vs 2.3 plain sizes), all four text encodings, tag-level and v2.4 frame-level unsynchronisation, the extended header, and refuses compressed/encrypted frames. Everything other than the genre frame still comes from `dhowden/tag`. Values are stored `;`-delimited — a delimiter the read side already splits on — so **no query changes** and the blast radius stays in the scanner. ## Repairing existing rows The fix alone would have repaired nothing. `scanFile`'s incremental skip short-circuits any file whose mtime hasn't moved, re-tagging doesn't move mtime, and no force-rescan affordance existed anywhere. Migration 0054 adds `tracks.tag_read_version` (`DEFAULT 0`) against `library.tagReadVersion = 1`, following the precedent already in that skip clause for `duration_ms = 0` backfill. Every existing row sorts below the constant and gets re-read on the next ordinary scan — no operator action, no rebuild. Bumping the constant is how a future extraction fix reaches an existing library, which is why it's a version rather than a `needs_reread` boolean. A version-driven re-read **reuses the stored duration** instead of re-running ffprobe, since the file's bytes haven't moved. That keeps a library-wide repair pass tag-read-bound rather than one fork+exec per file. `taste_profile_tags` needs nothing: `BuildTasteProfile` is an atomic delete-and-reinsert, so polluted rows are replaced on the next scheduled rebuild. ## Scope ID3v2 only. `dhowden` welds nowhere else — `readTFrame` is ID3v2-specific — so by construction every welded genre is an mp3. Confirmed independently by decoding the numerics against the ID3v1 table: `526617` = Electronic + New Wave + Rock, `1766Post-Punk43` = Rock + New Wave + Post-Punk + Punk. The Vorbis/MP4 repeated-field question is #2500, unproven and deliberately not built. ## Tests 20 cases in `internal/library/genre_test.go`, including the `Alternative RockRock` regression across all three major versions, the operator's 8-value blob, both UTF-16 BOM placements, Latin-1 widening, and the unsynchronisation collapse (with a guard that fails if the test ever goes vacuous). ## Also Corrected two comments that misattributed this bug: `recommendation/sessionvector.go` blamed "broken tag-editor output" — it was ours — and `api/library_browse.go` now warns that #2468's taxonomy question must only be judged against a re-scanned library. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01N6vZoJ4Se5YyaqdtGVkap5

bvandeusen added 2 commits 2026-08-05 22:10:31 -04:00

fix(scanner): read multi-value genre frames correctly — #2499 ... 37b396a7e4

dhowden/tag's readTFrame splits ID3v2 null-separated multi-value text
frames and rejoins them with the EMPTY string, so a file tagged
"Alternative Rock" + "Rock" was stored as "Alternative RockRock". It also
leaves bare numeric ID3v1 references unresolved, which is why the
library showed genres like "4017" and "526617".

This corrupted more than the browse axis added in #367: taste_profile.sql
reads tracks.genre directly, so the welded tokens were entering the taste
profile's tag vocabulary, and recommendation.sql/discover.sql were
comparing them as single opaque tags. Genre counts were wrong everywhere.

ffprobe is not a fix — ffmpeg's read_ttag calls decode_str once with no
loop, keeping only the first value. Truncating multi-genre tags would
blunt the similarity signal genre mainly feeds. So the TCON frame is now
parsed directly (ID3v2.2/2.3/2.4, all four text encodings, per-frame and
tag-level unsynchronisation, numeric and parenthesised ID3v1 references);
everything else still comes from dhowden/tag. Values are stored
";"-delimited, which the read side already splits on, so no query changes.

Existing rows are repaired without an operator-run rebuild: migration
0054 adds tracks.tag_read_version DEFAULT 0, below the scanner's current
tagReadVersion, so the next scan re-reads tags it would otherwise skip on
mtime. Such a re-read reuses the stored duration instead of re-running
ffprobe, keeping a repair pass tag-read-bound rather than one fork+exec
per file. Bumping the constant is how a future extraction fix reaches an
existing library.

Only ID3v2 is in scope — dhowden welds nowhere else. The Vorbis/MP4
repeated-field question is #2500, unproven and deliberately not built.

style(scanner): tagged switch on ID3 major version — #2499 fd27819cdd

bvandeusen merged commit 7e4727fc49 into main 2026-08-05 22:10:41 -04:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

deps

Dependency update (Renovate)

deps:major

Major dependency bump — held for manual review

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate-bot bvandeusen

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: bvandeusen/minstrel#120