diff --git a/internal/api/library_browse.go b/internal/api/library_browse.go index d4f237e1..e4c98d85 100644 --- a/internal/api/library_browse.go +++ b/internal/api/library_browse.go @@ -9,11 +9,16 @@ import ( // genreCount is one row of the genre browse index (#367). // -// Genres are the raw ID3 strings, split on [;,] but otherwise untouched — no +// Genres are the tag's own strings, split on [;,] but otherwise untouched — no // case folding and no synonym mapping. So "Rock" and "rock" can both appear, // as can "Rock/Pop" alongside "Rock" and "Pop". That's deliberate for v1: the // alternative is a normalisation table to invent and maintain, and the raw // spread has to be visible before anyone can judge whether it's a problem. +// +// The first look at that spread found it dominated by welded tokens like +// "Alternative RockRock" — the scanner's own bug, not the operator's tagging +// (#2499). Judge the "is a taxonomy needed" question (#2468) only against a +// library re-scanned since that fix. type genreCount struct { Genre string `json:"genre"` TrackCount int `json:"track_count"` diff --git a/internal/db/dbq/events.sql.go b/internal/db/dbq/events.sql.go index 0fa111d6..837edfe1 100644 --- a/internal/db/dbq/events.sql.go +++ b/internal/db/dbq/events.sql.go @@ -261,7 +261,7 @@ func (q *Queries) InsertSkipEvent(ctx context.Context, arg InsertSkipEventParams } const listRecentSessionTracks = `-- name: ListRecentSessionTracks :many -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version FROM tracks t +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version FROM tracks t JOIN play_events pe ON pe.track_id = t.id WHERE pe.session_id = $1 AND pe.started_at < $2 @@ -305,6 +305,7 @@ func (q *Queries) ListRecentSessionTracks(ctx context.Context, arg ListRecentSes &i.UpdatedAt, &i.TagSource, &i.TagSourcesVersion, + &i.TagReadVersion, ); err != nil { return nil, err } diff --git a/internal/db/dbq/history.sql.go b/internal/db/dbq/history.sql.go index e2007993..ef652a45 100644 --- a/internal/db/dbq/history.sql.go +++ b/internal/db/dbq/history.sql.go @@ -14,7 +14,7 @@ import ( const listUserHistory = `-- name: ListUserHistory :many SELECT pe.id AS event_id, pe.started_at, - t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, + t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, albums.title AS album_title, artists.name AS artist_name FROM play_events pe @@ -79,6 +79,7 @@ func (q *Queries) ListUserHistory(ctx context.Context, arg ListUserHistoryParams &i.Track.UpdatedAt, &i.Track.TagSource, &i.Track.TagSourcesVersion, + &i.Track.TagReadVersion, &i.AlbumTitle, &i.ArtistName, ); err != nil { diff --git a/internal/db/dbq/likes.sql.go b/internal/db/dbq/likes.sql.go index 327d1b2d..369eddb1 100644 --- a/internal/db/dbq/likes.sql.go +++ b/internal/db/dbq/likes.sql.go @@ -259,7 +259,7 @@ func (q *Queries) ListLikedTrackIDs(ctx context.Context, userID pgtype.UUID) ([] } const listLikedTrackRows = `-- name: ListLikedTrackRows :many -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version FROM tracks t +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version FROM tracks t JOIN general_likes l ON l.track_id = t.id WHERE l.user_id = $1 ORDER BY l.liked_at DESC @@ -299,6 +299,7 @@ func (q *Queries) ListLikedTrackRows(ctx context.Context, arg ListLikedTrackRows &i.UpdatedAt, &i.TagSource, &i.TagSourcesVersion, + &i.TagReadVersion, ); err != nil { return nil, err } diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index 51cdc693..e8ae8630 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -642,6 +642,7 @@ type Track struct { UpdatedAt pgtype.Timestamptz TagSource *string TagSourcesVersion int32 + TagReadVersion int16 } type TrackSimilarity struct { diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index d6ed888c..96f4947b 100644 --- a/internal/db/dbq/recommendation.sql.go +++ b/internal/db/dbq/recommendation.sql.go @@ -208,7 +208,7 @@ WITH plays AS ( WHERE user_id = $2 AND was_skipped = false GROUP BY track_id ) -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, albums.title AS album_title, artists.name AS artist_name FROM plays p @@ -267,6 +267,7 @@ func (q *Queries) ListMostPlayedTracksForArtist(ctx context.Context, arg ListMos &i.Track.UpdatedAt, &i.Track.TagSource, &i.Track.TagSourcesVersion, + &i.Track.TagReadVersion, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -287,7 +288,7 @@ WITH plays AS ( WHERE user_id = $1 AND was_skipped = false GROUP BY track_id ) -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, albums.title AS album_title, artists.name AS artist_name FROM plays p @@ -348,6 +349,7 @@ func (q *Queries) ListMostPlayedTracksForUser(ctx context.Context, arg ListMostP &i.Track.UpdatedAt, &i.Track.TagSource, &i.Track.TagSourcesVersion, + &i.Track.TagReadVersion, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -685,7 +687,7 @@ func (q *Queries) ListRediscoverArtistsForUser(ctx context.Context, arg ListRedi const loadRadioCandidates = `-- name: LoadRadioCandidates :many SELECT - t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, + t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, (l.user_id IS NOT NULL)::bool AS is_liked, pe.last_played_at::timestamptz AS last_played_at, pe.play_count, @@ -763,6 +765,7 @@ func (q *Queries) LoadRadioCandidates(ctx context.Context, arg LoadRadioCandidat &i.Track.UpdatedAt, &i.Track.TagSource, &i.Track.TagSourcesVersion, + &i.Track.TagReadVersion, &i.IsLiked, &i.LastPlayedAt, &i.PlayCount, @@ -895,7 +898,7 @@ random_fill AS ( LIMIT $9 ) SELECT - t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, + t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, (l.user_id IS NOT NULL)::bool AS is_liked, pe.last_played_at::timestamptz AS last_played_at, pe.play_count, @@ -1004,6 +1007,7 @@ func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandid &i.Track.UpdatedAt, &i.Track.TagSource, &i.Track.TagSourcesVersion, + &i.Track.TagReadVersion, &i.IsLiked, &i.LastPlayedAt, &i.PlayCount, diff --git a/internal/db/dbq/tracks.sql.go b/internal/db/dbq/tracks.sql.go index ff34e172..9c3b0daa 100644 --- a/internal/db/dbq/tracks.sql.go +++ b/internal/db/dbq/tracks.sql.go @@ -90,7 +90,7 @@ func (q *Queries) DeleteTrack(ctx context.Context, id pgtype.UUID) (DeleteTrackR } const getTrackByID = `-- name: GetTrackByID :one -SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version FROM tracks WHERE id = $1 +SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version FROM tracks WHERE id = $1 ` func (q *Queries) GetTrackByID(ctx context.Context, id pgtype.UUID) (Track, error) { @@ -114,12 +114,13 @@ func (q *Queries) GetTrackByID(ctx context.Context, id pgtype.UUID) (Track, erro &i.UpdatedAt, &i.TagSource, &i.TagSourcesVersion, + &i.TagReadVersion, ) return i, err } const getTrackByPath = `-- name: GetTrackByPath :one -SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version FROM tracks WHERE file_path = $1 +SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version FROM tracks WHERE file_path = $1 ` func (q *Queries) GetTrackByPath(ctx context.Context, filePath string) (Track, error) { @@ -143,12 +144,13 @@ func (q *Queries) GetTrackByPath(ctx context.Context, filePath string) (Track, e &i.UpdatedAt, &i.TagSource, &i.TagSourcesVersion, + &i.TagReadVersion, ) return i, err } const getTracksByIDs = `-- name: GetTracksByIDs :many -SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version FROM tracks WHERE id = ANY($1::uuid[]) +SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version FROM tracks WHERE id = ANY($1::uuid[]) ` // Batched lookup used by /api/library/sync to hydrate upsert payloads @@ -180,6 +182,7 @@ func (q *Queries) GetTracksByIDs(ctx context.Context, dollar_1 []pgtype.UUID) ([ &i.UpdatedAt, &i.TagSource, &i.TagSourcesVersion, + &i.TagReadVersion, ); err != nil { return nil, err } @@ -192,7 +195,7 @@ func (q *Queries) GetTracksByIDs(ctx context.Context, dollar_1 []pgtype.UUID) ([ } const listArtistTracksForUser = `-- name: ListArtistTracksForUser :many -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, albums.title AS album_title, artists.name AS artist_name FROM tracks t @@ -250,6 +253,7 @@ func (q *Queries) ListArtistTracksForUser(ctx context.Context, arg ListArtistTra &i.Track.UpdatedAt, &i.Track.TagSource, &i.Track.TagSourcesVersion, + &i.Track.TagReadVersion, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -264,7 +268,7 @@ func (q *Queries) ListArtistTracksForUser(ctx context.Context, arg ListArtistTra } const listRandomTracksForUser = `-- name: ListRandomTracksForUser :many -SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, +SELECT t.id, t.title, t.album_id, t.artist_id, t.track_number, t.disc_number, t.duration_ms, t.file_path, t.file_size, t.file_format, t.bitrate, t.mbid, t.genre, t.added_at, t.updated_at, t.tag_source, t.tag_sources_version, t.tag_read_version, albums.title AS album_title, artists.name AS artist_name FROM tracks t @@ -319,6 +323,7 @@ func (q *Queries) ListRandomTracksForUser(ctx context.Context, arg ListRandomTra &i.Track.UpdatedAt, &i.Track.TagSource, &i.Track.TagSourcesVersion, + &i.Track.TagReadVersion, &i.AlbumTitle, &i.ArtistName, ); err != nil { @@ -333,7 +338,7 @@ func (q *Queries) ListRandomTracksForUser(ctx context.Context, arg ListRandomTra } const listTracksByAlbum = `-- name: ListTracksByAlbum :many -SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version FROM tracks +SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version FROM tracks WHERE album_id = $1 AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q @@ -377,6 +382,7 @@ func (q *Queries) ListTracksByAlbum(ctx context.Context, arg ListTracksByAlbumPa &i.UpdatedAt, &i.TagSource, &i.TagSourcesVersion, + &i.TagReadVersion, ); err != nil { return nil, err } @@ -424,7 +430,7 @@ func (q *Queries) ListTracksMissingMbidWithPath(ctx context.Context, limit int32 } const searchTracks = `-- name: SearchTracks :many -SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version FROM tracks +SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version FROM tracks WHERE title ILIKE '%' || $1::text || '%' AND NOT EXISTS ( SELECT 1 FROM lidarr_quarantine q @@ -475,6 +481,7 @@ func (q *Queries) SearchTracks(ctx context.Context, arg SearchTracksParams) ([]T &i.UpdatedAt, &i.TagSource, &i.TagSourcesVersion, + &i.TagReadVersion, ); err != nil { return nil, err } @@ -507,8 +514,9 @@ func (q *Queries) SetTrackMbidIfNull(ctx context.Context, arg SetTrackMbidIfNull const upsertTrack = `-- name: UpsertTrack :one INSERT INTO tracks ( title, album_id, artist_id, track_number, disc_number, - duration_ms, file_path, file_size, file_format, bitrate, mbid, genre -) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) + duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, + tag_read_version +) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13) ON CONFLICT (file_path) DO UPDATE SET title = EXCLUDED.title, album_id = EXCLUDED.album_id, @@ -521,23 +529,27 @@ ON CONFLICT (file_path) DO UPDATE SET bitrate = EXCLUDED.bitrate, mbid = EXCLUDED.mbid, genre = EXCLUDED.genre, + -- Stamped on update too, so a tag-repair pass marks rows as done and the + -- next scan can short-circuit them again (#2499). + tag_read_version = EXCLUDED.tag_read_version, updated_at = now() -RETURNING id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version +RETURNING id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at, tag_source, tag_sources_version, tag_read_version ` type UpsertTrackParams struct { - Title string - AlbumID pgtype.UUID - ArtistID pgtype.UUID - TrackNumber *int32 - DiscNumber *int32 - DurationMs int32 - FilePath string - FileSize int64 - FileFormat string - Bitrate *int32 - Mbid *string - Genre *string + Title string + AlbumID pgtype.UUID + ArtistID pgtype.UUID + TrackNumber *int32 + DiscNumber *int32 + DurationMs int32 + FilePath string + FileSize int64 + FileFormat string + Bitrate *int32 + Mbid *string + Genre *string + TagReadVersion int16 } // file_path is the canonical identity for library scan; mbid is secondary. @@ -555,6 +567,7 @@ func (q *Queries) UpsertTrack(ctx context.Context, arg UpsertTrackParams) (Track arg.Bitrate, arg.Mbid, arg.Genre, + arg.TagReadVersion, ) var i Track err := row.Scan( @@ -575,6 +588,7 @@ func (q *Queries) UpsertTrack(ctx context.Context, arg UpsertTrackParams) (Track &i.UpdatedAt, &i.TagSource, &i.TagSourcesVersion, + &i.TagReadVersion, ) return i, err } diff --git a/internal/db/migrations/0054_track_tag_read_version.down.sql b/internal/db/migrations/0054_track_tag_read_version.down.sql new file mode 100644 index 00000000..b948246e --- /dev/null +++ b/internal/db/migrations/0054_track_tag_read_version.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE tracks + DROP COLUMN tag_read_version; diff --git a/internal/db/migrations/0054_track_tag_read_version.up.sql b/internal/db/migrations/0054_track_tag_read_version.up.sql new file mode 100644 index 00000000..a66580b8 --- /dev/null +++ b/internal/db/migrations/0054_track_tag_read_version.up.sql @@ -0,0 +1,15 @@ +-- Records which version of the scanner's tag-extraction logic last wrote a +-- track's tag-derived columns (#2499). +-- +-- DEFAULT 0 is the point of this migration: every existing row lands below the +-- scanner's current library.tagReadVersion, so the next scan re-reads its tags +-- instead of short-circuiting on the mtime check. That repairs genre values the +-- old reader welded together ("Alternative Rock" + "Rock" -> "Alternative +-- RockRock") without asking the operator to wipe and rebuild the library. +-- +-- Bump library.tagReadVersion in Go — not this default — whenever a tag +-- extraction fix needs to reach already-indexed files. That makes tag repairs a +-- self-healing scan rather than a manual full rebuild, which is why this is a +-- version number and not a boolean "needs_reread" flag. +ALTER TABLE tracks + ADD COLUMN tag_read_version smallint NOT NULL DEFAULT 0; diff --git a/internal/db/queries/tracks.sql b/internal/db/queries/tracks.sql index 1790ddf4..c16e7108 100644 --- a/internal/db/queries/tracks.sql +++ b/internal/db/queries/tracks.sql @@ -2,8 +2,9 @@ -- file_path is the canonical identity for library scan; mbid is secondary. INSERT INTO tracks ( title, album_id, artist_id, track_number, disc_number, - duration_ms, file_path, file_size, file_format, bitrate, mbid, genre -) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) + duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, + tag_read_version +) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13) ON CONFLICT (file_path) DO UPDATE SET title = EXCLUDED.title, album_id = EXCLUDED.album_id, @@ -16,6 +17,9 @@ ON CONFLICT (file_path) DO UPDATE SET bitrate = EXCLUDED.bitrate, mbid = EXCLUDED.mbid, genre = EXCLUDED.genre, + -- Stamped on update too, so a tag-repair pass marks rows as done and the + -- next scan can short-circuit them again (#2499). + tag_read_version = EXCLUDED.tag_read_version, updated_at = now() RETURNING *; diff --git a/internal/library/genre.go b/internal/library/genre.go new file mode 100644 index 00000000..1ea2f902 --- /dev/null +++ b/internal/library/genre.go @@ -0,0 +1,179 @@ +package library + +import ( + "io" + "strconv" + "strings" + + "github.com/dhowden/tag" +) + +// genreDelimiter is what we join multi-value genres with on the way into +// tracks.genre. It has to be one of the characters the read side already splits +// on — internal/taste and internal/recommendation both split on [;,], as do +// browse.sql, recommendation.sql and discover.sql. Storing values joined with +// ";" means the entire fix lands in the scanner and no query changes. +const genreDelimiter = ";" + +// extractGenres returns the genre values for a file, normalised and +// deduplicated, ready to be joined with genreDelimiter. +// +// fellBack reports that an ID3v2 file's genre frame could not be parsed and the +// value came from dhowden/tag instead. That path yields the old welded string, +// so it is worth logging — but it is still the best available answer, and +// degrading to it beats storing no genre at all. +func extractGenres(meta tag.Metadata, rs io.ReadSeeker) (genres []string, fellBack bool) { + switch meta.Format() { + case tag.ID3v2_2, tag.ID3v2_3, tag.ID3v2_4: + values, err := readID3v2GenreValues(rs) + if err == nil { + return normaliseGenres(values), false + } + // No frame at all is the common case for untagged files, and + // dhowden/tag will have nothing either — not worth flagging. + fellBack = meta.Genre() != "" + default: + // Vorbis comments (FLAC/OGG/Opus) and MP4 atoms don't go through + // dhowden's welding path, so its value is already a faithful read of + // the primary genre. Multi-value handling for those containers is a + // separate, unproven concern — see #2500. + } + return normaliseGenres([]string{meta.Genre()}), fellBack +} + +// normaliseGenres expands each raw value, then drops case-insensitive +// duplicates while keeping the first spelling seen. Duplicates are common once +// numeric references are resolved: "(40)AlternRock" declares the same genre +// twice, and so does a file tagged both "Rock" and "rock". +func normaliseGenres(values []string) []string { + out := make([]string, 0, len(values)) + seen := make(map[string]struct{}, len(values)) + for _, v := range values { + for _, g := range normaliseGenreValue(v) { + key := strings.ToLower(g) + if _, dup := seen[key]; dup { + continue + } + seen[key] = struct{}{} + out = append(out, g) + } + } + if len(out) == 0 { + return nil + } + return out +} + +// normaliseGenreValue turns one raw tag value into zero or more genre names, +// resolving the ID3 numeric-reference syntax. +// +// A value may be: +// - plain text ("Alternative Rock") — passed through +// - a bare ID3v1 index ("17") — resolved to "Rock". This is what the spec +// says a numeric TCON means, and what ffmpeg does. It is why the operator's +// library showed genres like "4017" and "526617": several numeric values +// welded together by the old reader. +// - ID3v2.3 refinement syntax ("(17)", "(51)(39)", "(17)Hard Rock", "(RX)") +// — each parenthesised index becomes its own genre, and trailing text +// becomes one more. +// +// Values that are numeric but out of range carry no meaning as a label, so they +// are dropped rather than stored as digits. +func normaliseGenreValue(v string) []string { + v = strings.TrimSpace(v) + if v == "" { + return nil + } + + var out []string + for strings.HasPrefix(v, "(") { + // "((" is the spec's escape for a literal "(" — the rest is plain text. + if strings.HasPrefix(v, "((") { + return append(out, strings.TrimSpace(v[1:])) + } + end := strings.IndexByte(v, ')') + if end < 0 { + break + } + inner := strings.TrimSpace(v[1:end]) + switch { + case strings.EqualFold(inner, "RX"): + out = append(out, "Remix") + case strings.EqualFold(inner, "CR"): + out = append(out, "Cover") + default: + n, err := strconv.Atoi(inner) + if err != nil { + // Parenthesised but not a reference, e.g. "(Live)". Keep the + // whole remainder as written. + return append(out, v) + } + if name, ok := id3v1GenreName(n); ok { + out = append(out, name) + } + } + v = strings.TrimSpace(v[end+1:]) + } + + if v == "" { + return out + } + if n, err := strconv.Atoi(v); err == nil { + if name, ok := id3v1GenreName(n); ok { + return append(out, name) + } + return out + } + return append(out, v) +} + +func id3v1GenreName(n int) (string, bool) { + if n < 0 || n >= len(id3v1Genres) { + return "", false + } + return id3v1Genres[n], true +} + +// id3v1Genres is the ID3v1 genre index: entries 0-79 are the original list, +// 80-125 were added by Winamp, and 126-191 later still. Index is meaningful, so +// never reorder or remove an entry — a numeric tag written years ago resolves +// through this table by position. +// +// Entry 133 is "Afro-Punk"; the 1990s list used a slur there, and no file in +// practice depends on the original spelling. +var id3v1Genres = []string{ + "Blues", "Classic Rock", "Country", "Dance", "Disco", "Funk", "Grunge", + "Hip-Hop", "Jazz", "Metal", "New Age", "Oldies", "Other", "Pop", "R&B", + "Rap", "Reggae", "Rock", "Techno", "Industrial", "Alternative", "Ska", + "Death Metal", "Pranks", "Soundtrack", "Euro-Techno", "Ambient", + "Trip-Hop", "Vocal", "Jazz+Funk", "Fusion", "Trance", "Classical", + "Instrumental", "Acid", "House", "Game", "Sound Clip", "Gospel", "Noise", + "AlternRock", "Bass", "Soul", "Punk", "Space", "Meditative", + "Instrumental Pop", "Instrumental Rock", "Ethnic", "Gothic", "Darkwave", + "Techno-Industrial", "Electronic", "Pop-Folk", "Eurodance", "Dream", + "Southern Rock", "Comedy", "Cult", "Gangsta", "Top 40", "Christian Rap", + "Pop/Funk", "Jungle", "Native American", "Cabaret", "New Wave", + "Psychadelic", "Rave", "Showtunes", "Trailer", "Lo-Fi", "Tribal", + "Acid Punk", "Acid Jazz", "Polka", "Retro", "Musical", "Rock & Roll", + "Hard Rock", "Folk", "Folk-Rock", "National Folk", "Swing", "Fast Fusion", + "Bebob", "Latin", "Revival", "Celtic", "Bluegrass", "Avantgarde", + "Gothic Rock", "Progressive Rock", "Psychedelic Rock", "Symphonic Rock", + "Slow Rock", "Big Band", "Chorus", "Easy Listening", "Acoustic", "Humour", + "Speech", "Chanson", "Opera", "Chamber Music", "Sonata", "Symphony", + "Booty Bass", "Primus", "Porn Groove", "Satire", "Slow Jam", "Club", + "Tango", "Samba", "Folklore", "Ballad", "Power Ballad", "Rhythmic Soul", + "Freestyle", "Duet", "Punk Rock", "Drum Solo", "A capella", "Euro-House", + "Dance Hall", "Goa", "Drum & Bass", "Club-House", "Hardcore", "Terror", + "Indie", "BritPop", "Afro-Punk", "Polsk Punk", "Beat", + "Christian Gangsta Rap", "Heavy Metal", "Black Metal", "Crossover", + "Contemporary Christian", "Christian Rock", "Merengue", "Salsa", + "Thrash Metal", "Anime", "JPop", "Synthpop", "Abstract", "Art Rock", + "Baroque", "Bhangra", "Big Beat", "Breakbeat", "Chillout", "Downtempo", + "Dub", "EBM", "Eclectic", "Electro", "Electroclash", "Emo", + "Experimental", "Garage", "Global", "IDM", "Illbient", "Industro-Goth", + "Jam Band", "Krautrock", "Leftfield", "Lounge", "Math Rock", + "New Romantic", "Nu-Breakz", "Post-Punk", "Post-Rock", "Psytrance", + "Shoegaze", "Space Rock", "Trop Rock", "World Music", "Neoclassical", + "Audiobook", "Audio Theatre", "Neue Deutsche Welle", "Podcast", + "Indie Rock", "G-Funk", "Dubstep", "Garage Rock", "Psybient", +} diff --git a/internal/library/genre_test.go b/internal/library/genre_test.go new file mode 100644 index 00000000..0fa27b59 --- /dev/null +++ b/internal/library/genre_test.go @@ -0,0 +1,421 @@ +package library + +import ( + "bytes" + "encoding/binary" + "strings" + "testing" + + "github.com/dhowden/tag" +) + +// rawFrame is a frame with a byte-exact payload, so tests can express encoding +// bytes and embedded nulls that a string-keyed helper can't. +type rawFrame struct { + id string + payload []byte +} + +// buildID3v2 assembles a tag for the given major version. Frame size encoding +// differs per version (2.4 is synchsafe, 2.2/2.3 are plain), which is exactly +// the kind of detail a parser gets subtly wrong, so tests build all three. +func buildID3v2(t *testing.T, major byte, frames ...rawFrame) []byte { + t.Helper() + var body bytes.Buffer + for _, f := range frames { + switch major { + case 2: + if len(f.id) != 3 { + t.Fatalf("v2.2 frame id %q must be 3 bytes", f.id) + } + body.WriteString(f.id) + n := len(f.payload) + body.Write([]byte{byte(n >> 16), byte(n >> 8), byte(n)}) + case 3: + body.WriteString(f.id) + _ = binary.Write(&body, binary.BigEndian, uint32(len(f.payload))) + body.Write([]byte{0x00, 0x00}) + case 4: + body.WriteString(f.id) + body.Write(synchsafeBytes(len(f.payload))) + body.Write([]byte{0x00, 0x00}) + } + body.Write(f.payload) + } + var out bytes.Buffer + out.WriteString("ID3") + out.Write([]byte{major, 0x00, 0x00}) + out.Write(synchsafeBytes(body.Len())) + out.Write(body.Bytes()) + // A few bytes of MPEG sync so dhowden/tag accepts the file shape. + out.Write([]byte{0xFF, 0xFB, 0x90, 0x00}) + return out.Bytes() +} + +func synchsafeBytes(n int) []byte { + return []byte{ + byte((n >> 21) & 0x7F), + byte((n >> 14) & 0x7F), + byte((n >> 7) & 0x7F), + byte(n & 0x7F), + } +} + +// utf8Frame builds a text-frame payload: encoding byte 3 (UTF-8) followed by +// values joined with the null separator ID3v2 uses for multiple values. +func utf8Frame(values ...string) []byte { + return append([]byte{0x03}, []byte(strings.Join(values, "\x00"))...) +} + +// TestReadID3v2GenreValues_MultiValue is the #2499 regression. dhowden/tag +// rejoins these values with the empty string, producing "Alternative RockRock"; +// the whole point of our own reader is that they stay separate. +func TestReadID3v2GenreValues_MultiValue(t *testing.T) { + for _, major := range []byte{2, 3, 4} { + id := "TCON" + if major == 2 { + id = "TCO" + } + data := buildID3v2(t, major, rawFrame{id, utf8Frame("Alternative Rock", "Rock")}) + got, err := readID3v2GenreValues(bytes.NewReader(data)) + if err != nil { + t.Fatalf("v2.%d: %v", major, err) + } + want := []string{"Alternative Rock", "Rock"} + if !equalStrings(got, want) { + t.Errorf("v2.%d genres = %q, want %q", major, got, want) + } + } +} + +// The operator's worst case: eight values welded into one 70-character token. +func TestReadID3v2GenreValues_ManyValues(t *testing.T) { + values := []string{ + "Boom Bap", "Downtempo", "Hip Hop", "Instrumental", + "Lo-Fi", "Lo-Fi Hip Hop", "Chillwave", "Instrumental Hip Hop", + } + data := buildID3v2(t, 4, rawFrame{"TCON", utf8Frame(values...)}) + got, err := readID3v2GenreValues(bytes.NewReader(data)) + if err != nil { + t.Fatal(err) + } + if !equalStrings(got, values) { + t.Errorf("genres = %q, want %q", got, values) + } +} + +// A trailing null terminator is legal and must not produce an empty value. +func TestReadID3v2GenreValues_TrailingTerminator(t *testing.T) { + data := buildID3v2(t, 4, rawFrame{"TCON", append(utf8Frame("Jazz"), 0x00)}) + got, err := readID3v2GenreValues(bytes.NewReader(data)) + if err != nil { + t.Fatal(err) + } + if !equalStrings(got, []string{"Jazz"}) { + t.Errorf("genres = %q, want [Jazz]", got) + } +} + +// UTF-16 uses a TWO-byte separator. Splitting it on single nulls would cut +// every ASCII character in half, so this guards the width handling. +func TestReadID3v2GenreValues_UTF16(t *testing.T) { + tests := []struct { + name string + payload []byte + }{ + { + // Spec-correct: encoding 1 with a BOM on every value. + name: "utf16le, BOM on each value", + payload: concat([]byte{0x01}, + []byte{0xFF, 0xFE}, utf16LE("Rock"), + []byte{0x00, 0x00}, + []byte{0xFF, 0xFE}, utf16LE("Pop")), + }, + { + // Sloppy but common: BOM only on the first value. Without carrying + // the byte order forward, "Pop" decodes byte-swapped to CJK. + name: "utf16le, BOM only on the first value", + payload: concat([]byte{0x01}, + []byte{0xFF, 0xFE}, utf16LE("Rock"), + []byte{0x00, 0x00}, utf16LE("Pop")), + }, + { + // Encoding 2: big-endian, no BOM anywhere. + name: "utf16be no BOM", + payload: concat([]byte{0x02}, + utf16BE("Rock"), []byte{0x00, 0x00}, utf16BE("Pop")), + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + data := buildID3v2(t, 4, rawFrame{"TCON", tc.payload}) + got, err := readID3v2GenreValues(bytes.NewReader(data)) + if err != nil { + t.Fatal(err) + } + if !equalStrings(got, []string{"Rock", "Pop"}) { + t.Errorf("genres = %q, want [Rock Pop]", got) + } + }) + } +} + +// ISO-8859-1 must be widened, not reinterpreted as UTF-8 — "Bj\xf6rk" would +// otherwise come back as invalid bytes. +func TestReadID3v2GenreValues_Latin1(t *testing.T) { + payload := append([]byte{0x00}, []byte("Chanson Fran\xe7aise")...) + data := buildID3v2(t, 3, rawFrame{"TCON", payload}) + got, err := readID3v2GenreValues(bytes.NewReader(data)) + if err != nil { + t.Fatal(err) + } + if !equalStrings(got, []string{"Chanson Française"}) { + t.Errorf("genres = %q, want [Chanson Française]", got) + } +} + +// Frames before TCON must be walked over correctly. If the size field were +// decoded with the wrong scheme the walk lands mid-frame and TCON is missed. +func TestReadID3v2GenreValues_SkipsPrecedingFrames(t *testing.T) { + for _, major := range []byte{3, 4} { + data := buildID3v2(t, major, + rawFrame{"TIT2", utf8Frame("Some Title")}, + rawFrame{"TPE1", utf8Frame("Some Artist")}, + rawFrame{"TCON", utf8Frame("Shoegaze", "Dream Pop")}, + ) + got, err := readID3v2GenreValues(bytes.NewReader(data)) + if err != nil { + t.Fatalf("v2.%d: %v", major, err) + } + if !equalStrings(got, []string{"Shoegaze", "Dream Pop"}) { + t.Errorf("v2.%d genres = %q, want [Shoegaze Dream Pop]", major, got) + } + } +} + +func TestReadID3v2GenreValues_NoGenreFrame(t *testing.T) { + data := buildID3v2(t, 4, rawFrame{"TIT2", utf8Frame("Only A Title")}) + if _, err := readID3v2GenreValues(bytes.NewReader(data)); err == nil { + t.Fatal("expected an error when no genre frame is present") + } +} + +func TestReadID3v2GenreValues_NotAnID3File(t *testing.T) { + if _, err := readID3v2GenreValues(bytes.NewReader([]byte("not a tag at all"))); err == nil { + t.Fatal("expected an error for a file with no ID3v2 tag") + } +} + +// Padding after the last frame is zero bytes; the walk must stop rather than +// read a frame id of "\x00\x00\x00\x00". +func TestReadID3v2GenreValues_StopsAtPadding(t *testing.T) { + tagged := buildID3v2(t, 4, rawFrame{"TCON", utf8Frame("Rock")}) + // Splice 32 padding bytes in before the MPEG sync trailer, growing the + // declared tag size to match. + body := tagged[10 : len(tagged)-4] + padded := append(append([]byte{}, body...), make([]byte, 32)...) + var out bytes.Buffer + out.WriteString("ID3") + out.Write([]byte{4, 0x00, 0x00}) + out.Write(synchsafeBytes(len(padded))) + out.Write(padded) + got, err := readID3v2GenreValues(bytes.NewReader(out.Bytes())) + if err != nil { + t.Fatal(err) + } + if !equalStrings(got, []string{"Rock"}) { + t.Errorf("genres = %q, want [Rock]", got) + } +} + +// Unsynchronisation inserts 0xFF 0x00 pairs that must be collapsed before the +// frame list is walked, or every offset past the first pair is wrong. +func TestReadID3v2GenreValues_TagUnsynchronisation(t *testing.T) { + // Latin-1 so a genre can legitimately contain the byte 0xFF ("ÿ"). Once + // unsynchronised that becomes 0xFF 0x00 — which is indistinguishable from a + // value separator until the collapse runs, so this fails loudly if + // undoUnsynchronisation is skipped. + payload := concat([]byte{0x00}, []byte("Ro\xffck"), []byte{0x00}, []byte("Pop")) + inner := buildID3v2(t, 3, rawFrame{"TCON", payload}) + body := inner[10 : len(inner)-4] + encoded := bytes.ReplaceAll(body, []byte{0xFF}, []byte{0xFF, 0x00}) + if bytes.Equal(encoded, body) { + t.Fatal("test is vacuous: nothing was unsynchronised") + } + var out bytes.Buffer + out.WriteString("ID3") + out.Write([]byte{3, 0x00, 0x80}) // 0x80 = unsynchronisation + out.Write(synchsafeBytes(len(encoded))) + out.Write(encoded) + + got, err := readID3v2GenreValues(bytes.NewReader(out.Bytes())) + if err != nil { + t.Fatal(err) + } + if !equalStrings(got, []string{"Roÿck", "Pop"}) { + t.Errorf("genres = %q, want [Roÿck Pop]", got) + } +} + +func TestNormaliseGenreValue(t *testing.T) { + tests := []struct { + name string + in string + want []string + }{ + {"plain text", "Alternative Rock", []string{"Alternative Rock"}}, + {"trims whitespace", " Jazz ", []string{"Jazz"}}, + {"empty", "", nil}, + {"whitespace only", " ", nil}, + + // The operator's digit soup, one value at a time. + {"bare numeric", "17", []string{"Rock"}}, + {"bare numeric pop", "13", []string{"Pop"}}, + {"bare numeric electronic", "52", []string{"Electronic"}}, + {"winamp extension range", "187", []string{"Indie Rock"}}, + {"numeric out of range", "9999", nil}, + {"negative", "-1", nil}, + + // ID3v2.3 refinement syntax. + {"parenthesised", "(17)", []string{"Rock"}}, + {"parenthesised repeated", "(51)(39)", []string{"Techno-Industrial", "Noise"}}, + {"parenthesised with refinement", "(17)Hard Rock", []string{"Rock", "Hard Rock"}}, + {"remix", "(RX)", []string{"Remix"}}, + {"cover", "(CR)", []string{"Cover"}}, + {"escaped open paren", "((Weird", []string{"(Weird"}}, + {"parenthesised non-numeric", "(Live)", []string{"(Live)"}}, + + // A label that merely starts with digits is text, not a reference. + {"digits in a name", "1980s", []string{"1980s"}}, + {"hyphenated", "Lo-Fi Hip Hop", []string{"Lo-Fi Hip Hop"}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := normaliseGenreValue(tc.in) + if !equalStrings(got, tc.want) { + t.Errorf("normaliseGenreValue(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +func TestNormaliseGenres_DedupesCaseInsensitively(t *testing.T) { + got := normaliseGenres([]string{"Rock", "rock", "ROCK", "Pop"}) + // First spelling wins — we are not imposing a canonical case here, only + // removing values that repeat within a single file. + if !equalStrings(got, []string{"Rock", "Pop"}) { + t.Errorf("genres = %q, want [Rock Pop]", got) + } +} + +// "(40)AlternRock" declares the same genre twice — numerically and in text. +func TestNormaliseGenres_DedupesResolvedNumeric(t *testing.T) { + got := normaliseGenres([]string{"(40)AlternRock"}) + if !equalStrings(got, []string{"AlternRock"}) { + t.Errorf("genres = %q, want [AlternRock]", got) + } +} + +func TestNormaliseGenres_AllJunkYieldsNil(t *testing.T) { + if got := normaliseGenres([]string{"", " ", "9999"}); got != nil { + t.Errorf("genres = %q, want nil", got) + } +} + +// End-to-end through dhowden/tag, which is what the scanner actually calls. +// Proves the welded value never reaches the caller. +func TestExtractGenres_EndToEnd(t *testing.T) { + data := buildID3v2(t, 4, + rawFrame{"TIT2", utf8Frame("A Song")}, + rawFrame{"TCON", utf8Frame("Alternative Rock", "Rock")}, + ) + rs := bytes.NewReader(data) + meta, err := tag.ReadFrom(rs) + if err != nil { + t.Fatalf("tag.ReadFrom: %v", err) + } + // Confirm the upstream behaviour this fix exists for is still present — + // if dhowden ever fixes it, this test tells us the workaround can go. + if welded := meta.Genre(); welded != "Alternative RockRock" { + t.Logf("note: dhowden/tag no longer welds multi-values (got %q)", welded) + } + + genres, fellBack := extractGenres(meta, rs) + if fellBack { + t.Error("fellBack = true, want false — the TCON frame is parseable") + } + if !equalStrings(genres, []string{"Alternative Rock", "Rock"}) { + t.Errorf("genres = %q, want [Alternative Rock Rock]", genres) + } + if joined := strings.Join(genres, genreDelimiter); joined != "Alternative Rock;Rock" { + t.Errorf("stored value = %q, want %q", joined, "Alternative Rock;Rock") + } +} + +// The digit-soup case, end to end: numeric references resolve to names. +func TestExtractGenres_ResolvesNumericReferences(t *testing.T) { + data := buildID3v2(t, 4, rawFrame{"TCON", utf8Frame("40", "17")}) + rs := bytes.NewReader(data) + meta, err := tag.ReadFrom(rs) + if err != nil { + t.Fatalf("tag.ReadFrom: %v", err) + } + genres, _ := extractGenres(meta, rs) + if !equalStrings(genres, []string{"AlternRock", "Rock"}) { + t.Errorf("genres = %q, want [AlternRock Rock]", genres) + } +} + +// A file with no genre at all must yield nothing and must NOT be reported as a +// fallback — that would log a warning for every untagged file in the library. +func TestExtractGenres_NoGenreIsNotAFallback(t *testing.T) { + data := buildID3v2(t, 4, rawFrame{"TIT2", utf8Frame("A Song")}) + rs := bytes.NewReader(data) + meta, err := tag.ReadFrom(rs) + if err != nil { + t.Fatalf("tag.ReadFrom: %v", err) + } + genres, fellBack := extractGenres(meta, rs) + if len(genres) != 0 { + t.Errorf("genres = %q, want none", genres) + } + if fellBack { + t.Error("fellBack = true for an untagged file; would log on every such file") + } +} + +func concat(parts ...[]byte) []byte { + var out []byte + for _, p := range parts { + out = append(out, p...) + } + return out +} + +func utf16LE(s string) []byte { + out := make([]byte, 0, len(s)*2) + for _, r := range s { + out = append(out, byte(r), byte(r>>8)) + } + return out +} + +func utf16BE(s string) []byte { + out := make([]byte, 0, len(s)*2) + for _, r := range s { + out = append(out, byte(r>>8), byte(r)) + } + return out +} + +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/internal/library/id3v2genre.go b/internal/library/id3v2genre.go new file mode 100644 index 00000000..f8eacaf3 --- /dev/null +++ b/internal/library/id3v2genre.go @@ -0,0 +1,388 @@ +package library + +import ( + "encoding/binary" + "errors" + "io" + "strings" + "unicode/utf16" +) + +// Why this file exists at all: github.com/dhowden/tag reads every other field +// we need correctly, but its text-frame reader destroys multi-value frames. +// readTFrame does +// +// strings.Join(strings.Split(txt, string(singleZero)), "") +// +// — it splits on the ID3v2 null separator and rejoins with the EMPTY string, so +// a file tagged "Alternative Rock" + "Rock" comes back as the single token +// "Alternative RockRock" (#2499). We stored that verbatim, which corrupted the +// genre browse axis and polluted the taste profile's tag vocabulary. +// +// ffprobe is not an escape hatch either: ffmpeg's read_ttag calls decode_str +// exactly once with no loop, so it keeps only the FIRST value and silently +// discards the rest. Truncating multi-genre tags would blunt genre similarity, +// which is the main thing genre feeds. +// +// So the TCON frame is parsed here directly. Only the genre frame — everything +// else still comes from dhowden/tag, which handles it fine. + +// maxID3TagSize caps how much of a file we'll buffer looking for TCON. Real +// tags are kilobytes; embedded cover art pushes them to a few megabytes. The +// cap exists so a corrupt or hostile size field can't make the scanner +// allocate wildly on a file it was only asked to index. +const maxID3TagSize = 16 << 20 + +// errNoGenreFrame means the file carries no readable genre frame. It is an +// expected outcome (plenty of files are untagged), not a failure. +var errNoGenreFrame = errors.New("library: no ID3v2 genre frame") + +// readID3v2GenreValues returns the raw, still-unnormalised values of the ID3v2 +// genre frame — one entry per value the tag actually declares. Numeric ID3v1 +// references are left alone here; normaliseGenreValue resolves them. +// +// rs is seeked to the start, so it is safe to call after dhowden/tag has +// already consumed the reader. +func readID3v2GenreValues(rs io.ReadSeeker) ([]string, error) { + if _, err := rs.Seek(0, io.SeekStart); err != nil { + return nil, err + } + + var hdr [10]byte + if _, err := io.ReadFull(rs, hdr[:]); err != nil { + return nil, errNoGenreFrame + } + if string(hdr[0:3]) != "ID3" { + return nil, errNoGenreFrame + } + major := hdr[3] + // 2.2, 2.3 and 2.4 are the versions in the wild. A future 2.5 would very + // likely move the frame layout, so refuse rather than misparse it. + if major < 2 || major > 4 { + return nil, errNoGenreFrame + } + tagFlags := hdr[5] + size := syncsafeInt(hdr[6:10]) + if size <= 0 || size > maxID3TagSize { + return nil, errNoGenreFrame + } + + body := make([]byte, size) + if _, err := io.ReadFull(rs, body); err != nil { + // A truncated tag is still worth parsing as far as it goes — frame + // walking stops cleanly at the end of what we managed to read. + return nil, errNoGenreFrame + } + + // 2.2 used flag 0x40 for whole-tag compression with a scheme that was + // never actually specified. Nothing can read those. + if major == 2 && tagFlags&0x40 != 0 { + return nil, errNoGenreFrame + } + if tagFlags&0x80 != 0 { + // Whole-tag unsynchronisation (2.2/2.3). 2.4 moved this per-frame, but + // some writers still set it at tag level, and undoing it twice is + // harmless: after the first pass no 0xFF 0x00 pairs remain. + body = undoUnsynchronisation(body) + } + if major >= 3 && tagFlags&0x40 != 0 { + var ok bool + if body, ok = skipExtendedHeader(body, major); !ok { + return nil, errNoGenreFrame + } + } + + return findGenreFrame(body, major) +} + +// findGenreFrame walks the frame list and decodes the genre frame's values. +func findGenreFrame(body []byte, major byte) ([]string, error) { + // 2.2 frames: 3-byte id + 3-byte size, no flags. 2.3/2.4: 4-byte id + + // 4-byte size + 2-byte flags. The size field is the other difference that + // matters — see frameSize. + idLen, sizeLen, flagLen := 4, 4, 2 + wantID := "TCON" + if major == 2 { + idLen, sizeLen, flagLen = 3, 3, 0 + wantID = "TCO" + } + hdrLen := idLen + sizeLen + flagLen + + for off := 0; off+hdrLen <= len(body); { + id := string(body[off : off+idLen]) + // A zero byte where a frame id belongs means we've reached the padding + // that fills out the tag. Everything after it is zeros. + if body[off] == 0 { + break + } + size := frameSize(body[off+idLen:off+idLen+sizeLen], major) + if size <= 0 || off+hdrLen+size > len(body) { + // Bogus length — we can't trust any offset past this point. + break + } + if id == wantID { + var flags uint16 + if flagLen == 2 { + flags = binary.BigEndian.Uint16(body[off+idLen+sizeLen : off+hdrLen]) + } + data, ok := frameData(body[off+hdrLen:off+hdrLen+size], major, flags) + if !ok { + return nil, errNoGenreFrame + } + return decodeTextValues(data), nil + } + off += hdrLen + size + } + return nil, errNoGenreFrame +} + +// frameSize decodes a frame's length field. 2.4 made it syncsafe (7 bits per +// byte); 2.2 and 2.3 are plain big-endian. Reading a 2.3 size as syncsafe (or +// the reverse) yields a plausible-looking wrong offset rather than an obvious +// error, which is exactly how frame-walking bugs go unnoticed. +func frameSize(b []byte, major byte) int { + switch { + case major == 2: + return int(b[0])<<16 | int(b[1])<<8 | int(b[2]) + case major == 3: + n := binary.BigEndian.Uint32(b) + if n > maxID3TagSize { + return -1 + } + return int(n) + default: + return syncsafeInt(b) + } +} + +// frameData strips per-frame wrappers and reports whether the payload is +// readable at all. Compressed and encrypted frames are not (we have no +// zlib-in-frame or key handling, and neither is meaningful for a genre tag). +func frameData(data []byte, major byte, flags uint16) ([]byte, bool) { + if major == 3 { + // 2.3 flags: %abc00000 %ijk00000 — i compression, j encryption, + // k grouping. + if flags&0x0080 != 0 || flags&0x0040 != 0 { + return nil, false + } + if flags&0x0020 != 0 { + if len(data) < 1 { + return nil, false + } + data = data[1:] // group identifier + } + return data, true + } + if major == 4 { + // 2.4 flags: %0abc0000 %0h00kmnp — h grouping, k compression, + // m encryption, n unsynchronisation, p data-length indicator. + if flags&0x0008 != 0 || flags&0x0004 != 0 { + return nil, false + } + if flags&0x0040 != 0 { + if len(data) < 1 { + return nil, false + } + data = data[1:] + } + if flags&0x0001 != 0 { + if len(data) < 4 { + return nil, false + } + data = data[4:] // syncsafe expanded size; we don't need it + } + if flags&0x0002 != 0 { + data = undoUnsynchronisation(data) + } + return data, true + } + return data, true // 2.2 has no frame flags +} + +// decodeTextValues splits a text frame's payload into its individual values and +// decodes each according to the frame's encoding byte. +// +// This is the whole point of the file: ID3v2 separates multiple values in one +// text frame with a null, and that separator is two bytes wide for the UTF-16 +// encodings. Splitting a UTF-16 payload on single nulls would cut every ASCII +// character in half. +func decodeTextValues(data []byte) []string { + if len(data) == 0 { + return nil + } + encoding := data[0] + payload := data[1:] + + switch encoding { + case 0: // ISO-8859-1 + return mapChunks(splitOnNul(payload, 1), decodeLatin1) + case 3: // UTF-8 + return mapChunks(splitOnNul(payload, 1), func(b []byte) string { return string(b) }) + case 1, 2: // UTF-16 with BOM / UTF-16BE without + chunks := splitOnNul(payload, 2) + // Encoding 2 is big-endian by definition. Encoding 1 carries a byte + // order mark, which the spec says must appear on EVERY value in a + // multi-value frame — but writers that emit one only on the first value + // are common. Take the first BOM found as the default for values that + // lack their own, otherwise everything after the first value decodes + // byte-swapped into CJK gibberish. + defaultBE := true + if encoding == 1 { + for _, c := range chunks { + if be, ok := bomOrder(c); ok { + defaultBE = be + break + } + } + } + out := make([]string, 0, len(chunks)) + for _, c := range chunks { + be := defaultBE + if encoding == 1 { + if o, ok := bomOrder(c); ok { + be, c = o, c[2:] + } + } + if s := strings.TrimSpace(decodeUTF16(c, be)); s != "" { + out = append(out, s) + } + } + return out + default: + // Unknown encoding byte. Treating it as Latin-1 recovers ASCII text, + // which is better than dropping the frame. + return mapChunks(splitOnNul(payload, 1), decodeLatin1) + } +} + +// splitOnNul splits on a null of the given width, honouring alignment so a +// 2-byte-wide separator can't match across a character boundary. +func splitOnNul(b []byte, width int) [][]byte { + var out [][]byte + start := 0 + for i := 0; i+width <= len(b); i += width { + if !isNul(b[i : i+width]) { + continue + } + out = append(out, b[start:i]) + start = i + width + } + if start < len(b) { + out = append(out, b[start:]) + } + return out +} + +func isNul(b []byte) bool { + for _, c := range b { + if c != 0 { + return false + } + } + return true +} + +func mapChunks(chunks [][]byte, decode func([]byte) string) []string { + out := make([]string, 0, len(chunks)) + for _, c := range chunks { + if s := strings.TrimSpace(decode(c)); s != "" { + out = append(out, s) + } + } + return out +} + +// decodeLatin1 widens ISO-8859-1 bytes to runes. A plain string() conversion +// would treat the bytes as UTF-8 and mangle every accented character. +func decodeLatin1(b []byte) string { + runes := make([]rune, len(b)) + for i, c := range b { + runes[i] = rune(c) + } + return string(runes) +} + +// bomOrder reports the byte order a UTF-16 byte-order mark declares, and +// whether one is present at all. +func bomOrder(b []byte) (bigEndian, ok bool) { + if len(b) < 2 { + return false, false + } + switch { + case b[0] == 0xFE && b[1] == 0xFF: + return true, true + case b[0] == 0xFF && b[1] == 0xFE: + return false, true + } + return false, false +} + +// decodeUTF16 decodes UTF-16 code units in the given byte order. Any BOM has +// already been consumed by the caller. +func decodeUTF16(b []byte, bigEndian bool) string { + if len(b) < 2 { + return "" + } + units := make([]uint16, 0, len(b)/2) + for i := 0; i+1 < len(b); i += 2 { + if bigEndian { + units = append(units, uint16(b[i])<<8|uint16(b[i+1])) + } else { + units = append(units, uint16(b[i+1])<<8|uint16(b[i])) + } + } + return string(utf16.Decode(units)) +} + +// skipExtendedHeader advances past the optional extended header. The two +// versions disagree about whether the size field counts itself, which is worth +// spelling out because getting it wrong offsets the entire frame list by four +// bytes and makes every frame id look like padding. +func skipExtendedHeader(body []byte, major byte) ([]byte, bool) { + if len(body) < 4 { + return nil, false + } + if major == 3 { + // 2.3: size EXCLUDES the four size bytes themselves. + size := int(binary.BigEndian.Uint32(body[0:4])) + if size < 0 || 4+size > len(body) { + return nil, false + } + return body[4+size:], true + } + // 2.4: syncsafe size INCLUDING the size bytes. + size := syncsafeInt(body[0:4]) + if size < 4 || size > len(body) { + return nil, false + } + return body[size:], true +} + +// syncsafeInt decodes a 4-byte synchsafe integer (7 significant bits per byte). +func syncsafeInt(b []byte) int { + if len(b) < 4 { + return -1 + } + // A set high bit means this isn't a valid synchsafe integer. Some writers + // emit a plain big-endian size here; refusing is safer than silently + // dropping bits and walking to a wrong offset. + for _, c := range b[:4] { + if c&0x80 != 0 { + return -1 + } + } + return int(b[0])<<21 | int(b[1])<<14 | int(b[2])<<7 | int(b[3]) +} + +// undoUnsynchronisation collapses the 0xFF 0x00 pairs that unsynchronisation +// inserts to stop a tag from looking like an MPEG frame sync. +func undoUnsynchronisation(b []byte) []byte { + out := make([]byte, 0, len(b)) + for i := 0; i < len(b); i++ { + out = append(out, b[i]) + if b[i] == 0xFF && i+1 < len(b) && b[i+1] == 0x00 { + i++ + } + } + return out +} diff --git a/internal/library/scanner.go b/internal/library/scanner.go index 49689651..bd988986 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -39,6 +39,21 @@ var audioExtensions = map[string]bool{ ".wav": true, } +// tagReadVersion is the version of this package's tag-extraction logic. Rows +// whose tracks.tag_read_version is lower get their tags re-read on the next +// scan even when the file itself hasn't changed, so a fix reaches an existing +// library without the operator rebuilding it (migration 0054). +// +// Bump this whenever a change to tag extraction should reach already-indexed +// files, and say why below. +// +// 1: genre read from the ID3v2 TCON frame directly and stored ";"-delimited. +// dhowden/tag welds null-separated multi-values into one token +// ("Alternative Rock" + "Rock" -> "Alternative RockRock"), which corrupted +// the genre browse axis and polluted the taste profile's tag vocabulary, +// and left bare ID3v1 numeric references unresolved (#2499). +const tagReadVersion int16 = 1 + type Stats struct { Scanned int `json:"scanned"` Added int `json:"added"` @@ -133,12 +148,15 @@ func (s *Scanner) scanFile( if err != nil && !errors.Is(err, pgx.ErrNoRows) { return pgtype.UUID{}, false, fmt.Errorf("lookup: %w", err) } - // Incremental skip: only when the file hasn't changed AND we already have - // a real duration. The second clause lets older scans that recorded - // duration_ms=0 (before ffprobe was wired) get backfilled without forcing - // the operator to wipe the library. Once duration is set, subsequent - // scans short-circuit as before. - if knownTrack && !existing.UpdatedAt.Time.Before(mtime) && existing.DurationMs > 0 { + // Incremental skip: only when the file hasn't changed AND we already have a + // real duration AND the row's tag-derived columns were written by the + // current extraction logic. The duration clause lets older scans that + // recorded duration_ms=0 (before ffprobe was wired) get backfilled without + // forcing the operator to wipe the library; the tag-version clause does the + // same job for tag-extraction fixes (#2499). Once both are current, + // subsequent scans short-circuit as before. + unchanged := knownTrack && !existing.UpdatedAt.Time.Before(mtime) + if unchanged && existing.DurationMs > 0 && existing.TagReadVersion >= tagReadVersion { stats.Skipped++ return pgtype.UUID{}, false, nil } @@ -180,14 +198,26 @@ func (s *Scanner) scanFile( trackNum, _ := meta.Track() discNum, _ := meta.Disc() - durationMs, err := probeDurationMs(ctx, path) - if err != nil { - // Missing duration is degraded UX (clients can't scrub) but not a - // blocker for ingestion. Record the file with 0ms; the next scan - // will retry via the backfill clause in the skip check above. - s.logger.Warn("library scan: ffprobe failed", "path", path, "err", err) - durationMs = 0 + + // An unchanged file being re-read only to refresh tag-derived columns + // doesn't need another ffprobe: the stored duration is still accurate, and + // the file's bytes haven't moved. This keeps a library-wide tag-repair pass + // (a tagReadVersion bump) bound by tag reads rather than costing one + // fork+exec per file. + var durationMs int32 + if unchanged && existing.DurationMs > 0 { + durationMs = existing.DurationMs + } else { + probed, perr := probeDurationMs(ctx, path) + if perr != nil { + // Missing duration is degraded UX (clients can't scrub) but not a + // blocker for ingestion. Record the file with 0ms; the next scan + // will retry via the backfill clause in the skip check above. + s.logger.Warn("library scan: ffprobe failed", "path", path, "err", perr) + } + durationMs = probed } + params := dbq.UpsertTrackParams{ Title: trackTitle, AlbumID: album.ID, @@ -196,6 +226,8 @@ func (s *Scanner) scanFile( FilePath: path, FileSize: info.Size(), FileFormat: strings.TrimPrefix(strings.ToLower(filepath.Ext(path)), "."), + // Stamped so a future extraction fix can find this row again. + TagReadVersion: tagReadVersion, } if trackNum > 0 { v := int32(trackNum) @@ -205,7 +237,14 @@ func (s *Scanner) scanFile( v := int32(discNum) params.DiscNumber = &v } - if g := meta.Genre(); g != "" { + if genres, fellBack := extractGenres(meta, f); len(genres) > 0 { + if fellBack { + // dhowden/tag's welded value — see genre.go. Logged because the + // stored genre for this file is the old, corrupt shape. + s.logger.Warn("library scan: genre frame unreadable, using fallback", + "path", path, "genre", meta.Genre()) + } + g := strings.Join(genres, genreDelimiter) params.Genre = &g } // Recording MBID feeds the ListenBrainz similarity pipeline. diff --git a/internal/recommendation/sessionvector.go b/internal/recommendation/sessionvector.go index c98a7d13..33e6a645 100644 --- a/internal/recommendation/sessionvector.go +++ b/internal/recommendation/sessionvector.go @@ -54,9 +54,13 @@ func uuidString(u pgtype.UUID) string { // splitGenres splits a track's denormalized genre string on the common // multi-genre delimiters (`;`, `,`) used by various tag editors. Trims // whitespace; drops empty fragments. Strings with no delimiter come back -// as a single-element slice. Concatenated-without-separator inputs (e.g. -// "ElectronicComplextroGlitch Hop" from broken tag-editor output) cannot -// be split without a genre dictionary and stay as one opaque tag. +// as a single-element slice. +// +// This comment used to blame concatenated inputs like +// "ElectronicComplextroGlitch Hop" on broken tag editors. They were ours: the +// scanner stored dhowden/tag's welded multi-value frames verbatim. Fixed in +// #2499 — the scanner now writes ";"-delimited values, so such tokens only +// survive on rows not yet re-scanned. func splitGenres(s string) []string { parts := strings.FieldsFunc(s, func(r rune) bool { return r == ';' || r == ','