diff --git a/internal/db/dbq/system_playlists.sql.go b/internal/db/dbq/system_playlists.sql.go index f76d10ff..d8fa1ae4 100644 --- a/internal/db/dbq/system_playlists.sql.go +++ b/internal/db/dbq/system_playlists.sql.go @@ -452,6 +452,7 @@ SELECT COALESCE( FROM tracks t JOIN albums a ON a.id = t.album_id WHERE t.artist_id = $2 + AND t.missing_since IS NULL -- #2701: the play branch filters; this one must too ORDER BY a.release_date DESC NULLS LAST, t.disc_number NULLS LAST, t.track_number NULLS LAST, @@ -497,6 +498,7 @@ alltime AS ( liked AS ( SELECT gl.track_id AS id, 0::bigint AS c, 2 AS tier FROM general_likes gl + JOIN tracks t ON t.id = gl.track_id AND t.missing_since IS NULL WHERE gl.user_id = $1 ), chosen AS ( @@ -526,6 +528,10 @@ SELECT id // Widened from a hard 7-day window, which made For-You disappear // after a week of not listening and never recover on a self-hosted // library with sparse history. +// A likes-only read still joins tracks: a like outlives its file, and a +// seed whose file is gone points For You at something the user can't hear +// (#2701). The other two tiers get the same filter from their play_events +// join. func (q *Queries) PickTopPlayedTracksForUser(ctx context.Context, userID pgtype.UUID) ([]pgtype.UUID, error) { rows, err := q.db.Query(ctx, pickTopPlayedTracksForUser, userID) if err != nil { diff --git a/internal/db/queries/system_playlists.sql b/internal/db/queries/system_playlists.sql index 41c1cff5..7c97a7a1 100644 --- a/internal/db/queries/system_playlists.sql +++ b/internal/db/queries/system_playlists.sql @@ -161,9 +161,14 @@ alltime AS ( AND pe.was_skipped = false GROUP BY t.id ), +-- A likes-only read still joins tracks: a like outlives its file, and a +-- seed whose file is gone points For You at something the user can't hear +-- (#2701). The other two tiers get the same filter from their play_events +-- join. liked AS ( SELECT gl.track_id AS id, 0::bigint AS c, 2 AS tier FROM general_likes gl + JOIN tracks t ON t.id = gl.track_id AND t.missing_since IS NULL WHERE gl.user_id = $1 ), chosen AS ( @@ -201,6 +206,7 @@ SELECT COALESCE( FROM tracks t JOIN albums a ON a.id = t.album_id WHERE t.artist_id = $2 + AND t.missing_since IS NULL -- #2701: the play branch filters; this one must too ORDER BY a.release_date DESC NULLS LAST, t.disc_number NULLS LAST, t.track_number NULLS LAST, diff --git a/internal/library/genre.go b/internal/library/genre.go index 2c903f13..7981e026 100644 --- a/internal/library/genre.go +++ b/internal/library/genre.go @@ -32,11 +32,25 @@ func extractGenres(meta tag.Metadata, rs io.ReadSeeker) (genres []string, fellBa // 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. + case tag.VORBIS, tag.MP4: + // dhowden/tag keeps only the last of a repeated field here (#2500): + // see vorbisgenre.go and mp4genre.go. + var values []string + var err error + switch { + case meta.Format() == tag.MP4: + values, err = readMP4GenreValues(rs) + case meta.FileType() == tag.OGG: + values, err = readOggGenreValues(rs) + default: + values, err = readFLACGenreValues(rs) + } + if err == nil && len(values) > 0 { + return normaliseGenres(values), false + } + // err == nil with no values: the file declares no genre of its own + // (or, for MP4, only the numeric gnre atom, which dhowden resolves). + fellBack = err != nil && meta.Genre() != "" } return normaliseGenres([]string{meta.Genre()}), fellBack } diff --git a/internal/library/mp4genre.go b/internal/library/mp4genre.go new file mode 100644 index 00000000..d2096e12 --- /dev/null +++ b/internal/library/mp4genre.go @@ -0,0 +1,168 @@ +package library + +import ( + "encoding/binary" + "errors" + "io" + "strings" +) + +// MP4 has the same gap as Vorbis comments (#2500). A tag writer stores several +// genres as several "data" atoms inside one ©gen atom (mutagen and Picard do), +// or occasionally as repeated ©gen atoms. dhowden/tag keeps atoms in a map keyed +// by name (mp4.go: m.data[name] = data), so one value survives either way. +// +// Path to the values: moov > udta > meta > ilst > ©gen > data. A file that +// carries only the numeric "gnre" atom has no ©gen; this reader returns +// (nil, nil) for it, and extractGenres falls back to dhowden/tag, which resolves +// gnre to a name. + +// maxMP4GenreAtomSize caps how much of one ©gen atom is buffered. Genre text is +// bytes; the cap only stops a corrupt size from making the scanner allocate. +const maxMP4GenreAtomSize = 1 << 20 + +var errMalformedAtom = errors.New("library: malformed MP4 atom") + +type mp4Atom struct { + kind string + body, limit int64 // body start and end offsets in the file +} + +// readMP4Atom reads the atom header at pos, which must lie within [pos, limit). +func readMP4Atom(rs io.ReadSeeker, pos, limit int64) (mp4Atom, error) { + if _, err := rs.Seek(pos, io.SeekStart); err != nil { + return mp4Atom{}, err + } + var hdr [8]byte + if _, err := io.ReadFull(rs, hdr[:]); err != nil { + return mp4Atom{}, errMalformedAtom + } + size := int64(binary.BigEndian.Uint32(hdr[0:4])) + hdrLen := int64(8) + switch size { + case 0: // runs to the end of the enclosing atom + size = limit - pos + case 1: // 64-bit size follows the type + var large [8]byte + if _, err := io.ReadFull(rs, large[:]); err != nil { + return mp4Atom{}, errMalformedAtom + } + size = int64(binary.BigEndian.Uint64(large[:])) + hdrLen = 16 + } + if size < hdrLen || size > limit-pos { + return mp4Atom{}, errMalformedAtom + } + return mp4Atom{kind: string(hdr[4:8]), body: pos + hdrLen, limit: pos + size}, nil +} + +// findMP4Child returns the first child of the given kind in [start, end). +func findMP4Child(rs io.ReadSeeker, start, end int64, kind string) (mp4Atom, bool, error) { + for pos := start; pos+8 <= end; { + a, err := readMP4Atom(rs, pos, end) + if err != nil { + return mp4Atom{}, false, err + } + if a.kind == kind { + return a, true, nil + } + pos = a.limit + } + return mp4Atom{}, false, nil +} + +// readMP4GenreValues returns every ©gen value, in file order. rs is seeked +// as needed, so it is safe to call after dhowden/tag has consumed the reader. +func readMP4GenreValues(rs io.ReadSeeker) ([]string, error) { + end, err := rs.Seek(0, io.SeekEnd) + if err != nil { + return nil, err + } + // moov may sit after mdat; walking headers and seeking past bodies finds + // it either way without reading the audio. + cur := mp4Atom{body: 0, limit: end} + for _, kind := range []string{"moov", "udta", "meta"} { + next, found, err := findMP4Child(rs, cur.body, cur.limit, kind) + if err != nil { + return nil, errNoGenreFrame + } + if !found { + return nil, nil // no iTunes metadata at all + } + cur = next + } + // meta is a "full box" with four bytes of version and flags before its + // children in the ISO layout. QuickTime-style files omit them, so the + // first child (hdlr) starts straight away. Look before skipping. + if _, err := rs.Seek(cur.body, io.SeekStart); err != nil { + return nil, errNoGenreFrame + } + var peek [8]byte + if _, err := io.ReadFull(rs, peek[:]); err != nil { + return nil, errNoGenreFrame + } + if string(peek[4:8]) != "hdlr" { + cur.body += 4 + } + ilst, found, err := findMP4Child(rs, cur.body, cur.limit, "ilst") + if err != nil { + return nil, errNoGenreFrame + } + if !found { + return nil, nil + } + + var out []string + for pos := ilst.body; pos+8 <= ilst.limit; { + a, err := readMP4Atom(rs, pos, ilst.limit) + if err != nil { + return nil, errNoGenreFrame + } + pos = a.limit + if a.kind != "\xa9gen" { + continue + } + n := a.limit - a.body + if n > maxMP4GenreAtomSize { + return nil, errNoGenreFrame + } + if _, err := rs.Seek(a.body, io.SeekStart); err != nil { + return nil, errNoGenreFrame + } + body := make([]byte, n) + if _, err := io.ReadFull(rs, body); err != nil { + return nil, errNoGenreFrame + } + values, ok := mp4DataValues(body) + if !ok { + return nil, errNoGenreFrame + } + out = append(out, values...) + } + return out, nil +} + +// mp4DataValues reads the "data" atoms in a ©gen body. Each is: size, "data", +// a 4-byte type indicator (1 = UTF-8 text), a 4-byte locale, then the value. +// Atoms of other types carry no text and are skipped. +func mp4DataValues(b []byte) ([]string, bool) { + var out []string + for len(b) >= 8 { + size := binary.BigEndian.Uint32(b[0:4]) + if size < 8 || uint64(size) > uint64(len(b)) { + return nil, false + } + atom := b[:size] + b = b[size:] + if string(atom[4:8]) != "data" || len(atom) < 16 { + continue + } + if binary.BigEndian.Uint32(atom[8:12])&0x00FFFFFF != 1 { + continue + } + if v := strings.TrimSpace(string(atom[16:])); v != "" { + out = append(out, v) + } + } + return out, true +} diff --git a/internal/library/scanner.go b/internal/library/scanner.go index e737aab5..4fcd73c9 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -60,7 +60,11 @@ var audioExtensions = map[string]bool{ // (#5241). Lidarr names albums by release group, not by the release id // albums.mbid holds, so re-acquisition and request completion need it for // the albums already in the library, not only for files added from now on. -const tagReadVersion int16 = 3 +// 4: every value of a repeated genre field is read for FLAC, Ogg Vorbis, Opus +// and MP4 (#2500). dhowden/tag kept only the last, and 3,658 of the +// operator's 4,092 FLACs declare more than one, so most of their genres +// were never stored. +const tagReadVersion int16 = 4 type Stats struct { Scanned int `json:"scanned"` diff --git a/internal/library/vorbisgenre.go b/internal/library/vorbisgenre.go new file mode 100644 index 00000000..fa3027ff --- /dev/null +++ b/internal/library/vorbisgenre.go @@ -0,0 +1,214 @@ +package library + +import ( + "bufio" + "bytes" + "encoding/binary" + "errors" + "io" + "strings" +) + +// Why this file exists: Vorbis comments (FLAC, Ogg Vorbis, Opus) say a field +// has several values by repeating it: +// +// GENRE=Boom Bap +// GENRE=Downtempo +// +// dhowden/tag stores comments in a map keyed by field name +// (vorbis.go: m.c[strings.ToLower(k)] = v), so each repeat overwrites the last +// and only the final value survives. On the operator's library that was 3,658 +// of 4,092 FLAC files: Kupla's Life Forms declares eight genres and was stored +// as "Instrumental Hip Hop" alone (#2500). Nothing looked corrupt; the other +// seven were simply never seen by browse or the taste profile. +// +// The comment block is read here directly, as the ID3v2 genre frame is in +// id3v2genre.go. Every other field still comes from dhowden/tag. + +// maxVorbisCommentSize caps a comment block or packet. Comments are kilobytes; +// an embedded METADATA_BLOCK_PICTURE can take them to a few megabytes. The cap +// only stops a corrupt length from making the scanner allocate wildly. +const maxVorbisCommentSize = 16 << 20 + +var errMalformedComment = errors.New("library: malformed Vorbis comment") + +// readFLACGenreValues returns every GENRE value in a FLAC file's comment block. +// (nil, nil) means the file is readable and declares no genre. rs is seeked to +// the start, so it is safe to call after dhowden/tag has consumed the reader. +func readFLACGenreValues(rs io.ReadSeeker) ([]string, error) { + if _, err := rs.Seek(0, io.SeekStart); err != nil { + return nil, err + } + var magic [4]byte + if _, err := io.ReadFull(rs, magic[:]); err != nil { + return nil, errNoGenreFrame + } + // Some taggers prepend an ID3v2 tag to a FLAC. The FLAC stream starts after it. + if string(magic[:3]) == "ID3" { + var rest [6]byte + if _, err := io.ReadFull(rs, rest[:]); err != nil { + return nil, errNoGenreFrame + } + size := syncsafeInt(rest[2:6]) + if size < 0 { + return nil, errNoGenreFrame + } + skip := int64(10 + size) + if rest[1]&0x10 != 0 { + skip += 10 // 2.4 footer + } + if _, err := rs.Seek(skip, io.SeekStart); err != nil { + return nil, errNoGenreFrame + } + if _, err := io.ReadFull(rs, magic[:]); err != nil { + return nil, errNoGenreFrame + } + } + if string(magic[:]) != "fLaC" { + return nil, errNoGenreFrame + } + + for { + var hdr [4]byte + if _, err := io.ReadFull(rs, hdr[:]); err != nil { + return nil, errNoGenreFrame + } + last := hdr[0]&0x80 != 0 + blockType := hdr[0] & 0x7F + size := int64(hdr[1])<<16 | int64(hdr[2])<<8 | int64(hdr[3]) + switch blockType { + case 4: // VORBIS_COMMENT + body := make([]byte, size) + if _, err := io.ReadFull(rs, body); err != nil { + return nil, errNoGenreFrame + } + return vorbisCommentGenres(body) + case 127: // invalid by spec; nothing after it can be trusted + return nil, errNoGenreFrame + } + if last { + return nil, nil // no comment block at all + } + if _, err := rs.Seek(size, io.SeekCurrent); err != nil { + return nil, errNoGenreFrame + } + } +} + +// readOggGenreValues returns every GENRE value in an Ogg Vorbis or Opus file. +// The comments are the stream's second packet, which can span several pages +// when it carries cover art, so pages are reassembled until it is complete. +// Only the first logical stream is read; pages of any other are skipped. +func readOggGenreValues(rs io.ReadSeeker) ([]string, error) { + if _, err := rs.Seek(0, io.SeekStart); err != nil { + return nil, err + } + br := bufio.NewReader(rs) + var ( + serial uint32 + haveFirst bool + packet []byte + packetIdx int + ) + for { + var hdr [27]byte + if _, err := io.ReadFull(br, hdr[:]); err != nil { + return nil, errNoGenreFrame + } + if string(hdr[0:4]) != "OggS" { + return nil, errNoGenreFrame + } + pageSerial := binary.LittleEndian.Uint32(hdr[14:18]) + segments := make([]byte, hdr[26]) + if _, err := io.ReadFull(br, segments); err != nil { + return nil, errNoGenreFrame + } + if !haveFirst { + serial, haveFirst = pageSerial, true + } + if pageSerial != serial { + var n int64 + for _, l := range segments { + n += int64(l) + } + if _, err := io.CopyN(io.Discard, br, n); err != nil { + return nil, errNoGenreFrame + } + continue + } + for _, l := range segments { + if len(packet)+int(l) > maxVorbisCommentSize { + return nil, errNoGenreFrame + } + seg := make([]byte, l) + if _, err := io.ReadFull(br, seg); err != nil { + return nil, errNoGenreFrame + } + packet = append(packet, seg...) + if l == 255 { + continue // the packet carries on in the next segment + } + if packetIdx == 1 { + return oggCommentGenres(packet) + } + packetIdx++ + packet = packet[:0] + } + } +} + +// oggCommentGenres strips the codec's comment-packet prefix. Ogg FLAC and Speex +// are rare enough to leave to dhowden/tag. +func oggCommentGenres(packet []byte) ([]string, error) { + switch { + case bytes.HasPrefix(packet, []byte("\x03vorbis")): + return vorbisCommentGenres(packet[7:]) + case bytes.HasPrefix(packet, []byte("OpusTags")): + return vorbisCommentGenres(packet[8:]) + } + return nil, errNoGenreFrame +} + +// vorbisCommentGenres reads a comment block (vendor string, then a count of +// length-prefixed "KEY=value" entries, little-endian lengths throughout) and +// returns the GENRE values in file order. Field names are case-insensitive by +// spec. Anything after the last entry, such as Vorbis's framing bit, is ignored. +func vorbisCommentGenres(b []byte) ([]string, error) { + next := func() ([]byte, bool) { + if len(b) < 4 { + return nil, false + } + n := binary.LittleEndian.Uint32(b) + if uint64(n) > uint64(len(b)-4) { + return nil, false + } + field := b[4 : 4+n] + b = b[4+n:] + return field, true + } + if _, ok := next(); !ok { // vendor string + return nil, errMalformedComment + } + if len(b) < 4 { + return nil, errMalformedComment + } + count := binary.LittleEndian.Uint32(b) + b = b[4:] + var out []string + // Each entry costs at least four bytes, so a corrupt count runs out of + // input long before it runs up a loop. + for i := uint32(0); i < count; i++ { + field, ok := next() + if !ok { + return nil, errMalformedComment + } + key, value, found := strings.Cut(string(field), "=") + if !found || !strings.EqualFold(key, "GENRE") { + continue + } + if value = strings.TrimSpace(value); value != "" { + out = append(out, value) + } + } + return out, nil +} diff --git a/internal/library/vorbisgenre_test.go b/internal/library/vorbisgenre_test.go new file mode 100644 index 00000000..1fe0f0be --- /dev/null +++ b/internal/library/vorbisgenre_test.go @@ -0,0 +1,288 @@ +package library + +import ( + "bytes" + "encoding/binary" + "strings" + "testing" + + "github.com/dhowden/tag" +) + +// vorbisComment builds a comment block: vendor, count, then each "KEY=value". +func vorbisComment(fields ...string) []byte { + var b bytes.Buffer + vendor := "minstrel-test" + _ = binary.Write(&b, binary.LittleEndian, uint32(len(vendor))) + b.WriteString(vendor) + _ = binary.Write(&b, binary.LittleEndian, uint32(len(fields))) + for _, f := range fields { + _ = binary.Write(&b, binary.LittleEndian, uint32(len(f))) + b.WriteString(f) + } + return b.Bytes() +} + +// buildFLAC assembles a STREAMINFO block and a comment block, optionally +// behind an ID3v2 tag the way some taggers write them. +func buildFLAC(withID3 bool, fields ...string) []byte { + var b bytes.Buffer + if withID3 { + pad := make([]byte, 20) + b.WriteString("ID3") + b.Write([]byte{4, 0, 0}) + b.Write(synchsafeBytes(len(pad))) + b.Write(pad) + } + b.WriteString("fLaC") + b.Write([]byte{0x00, 0, 0, 34}) // STREAMINFO, not last + b.Write(make([]byte, 34)) + vc := vorbisComment(fields...) + b.Write([]byte{0x84, byte(len(vc) >> 16), byte(len(vc) >> 8), byte(len(vc))}) // last, type 4 + b.Write(vc) + return b.Bytes() +} + +// oggCRC is the Ogg page checksum: CRC-32, polynomial 0x04c11db7, no +// reflection, zero initial value. dhowden/tag verifies it, so the end-to-end +// tests need real pages. +func oggCRC(p []byte) uint32 { + var crc uint32 + for _, c := range p { + crc ^= uint32(c) << 24 + for i := 0; i < 8; i++ { + if crc&0x80000000 != 0 { + crc = crc<<1 ^ 0x04c11db7 + } else { + crc <<= 1 + } + } + } + return crc +} + +// buildOgg lays packets out as Ogg pages for one logical stream, splitting a +// packet across pages when it needs more than 255 lacing values. +func buildOgg(serial uint32, packets ...[]byte) []byte { + var out bytes.Buffer + var seq uint32 + for _, p := range packets { + var lacing []byte + for n := len(p); ; n -= 255 { + if n >= 255 { + lacing = append(lacing, 255) + continue + } + lacing = append(lacing, byte(n)) + break + } + continued := false + for off := 0; len(lacing) > 0; { + segs := lacing + if len(segs) > 255 { + segs = segs[:255] + } + lacing = lacing[len(segs):] + n := 0 + for _, s := range segs { + n += int(s) + } + var page bytes.Buffer + page.WriteString("OggS") + page.WriteByte(0) + flags := byte(0) + if continued { + flags |= 0x01 + } + if seq == 0 { + flags |= 0x02 + } + page.WriteByte(flags) + page.Write(make([]byte, 8)) // granule position + _ = binary.Write(&page, binary.LittleEndian, serial) + _ = binary.Write(&page, binary.LittleEndian, seq) + page.Write(make([]byte, 4)) // checksum, filled below + page.WriteByte(byte(len(segs))) + page.Write(segs) + page.Write(p[off : off+n]) + raw := page.Bytes() + binary.LittleEndian.PutUint32(raw[22:26], oggCRC(raw)) + out.Write(raw) + off += n + seq++ + continued = true + } + } + return out.Bytes() +} + +func vorbisIdent() []byte { return append([]byte("\x01vorbis"), make([]byte, 23)...) } + +// mp4Box wraps children in an atom of the given kind. +func mp4Box(kind string, children ...[]byte) []byte { + body := bytes.Join(children, nil) + b := make([]byte, 8, 8+len(body)) + binary.BigEndian.PutUint32(b, uint32(8+len(body))) + copy(b[4:], kind) + return append(b, body...) +} + +func mp4Text(value string) []byte { + return mp4Box("data", []byte{0, 0, 0, 1, 0, 0, 0, 0}, []byte(value)) +} + +// buildMP4 places the ©gen atoms under moov/udta/meta/ilst. isoMeta adds the +// four version/flags bytes ISO files carry before meta's children. +func buildMP4(isoMeta bool, genAtoms ...[]byte) []byte { + ilst := mp4Box("ilst", append([][]byte{mp4Box("\xa9nam", mp4Text("A Song"))}, genAtoms...)...) + hdlr := mp4Box("hdlr", make([]byte, 25)) + var meta []byte + if isoMeta { + meta = mp4Box("meta", []byte{0, 0, 0, 0}, hdlr, ilst) + } else { + meta = mp4Box("meta", hdlr, ilst) + } + ftyp := mp4Box("ftyp", []byte("M4A \x00\x00\x00\x00M4A mp42isom")) + mdat := mp4Box("mdat", make([]byte, 64)) + // moov after mdat, as many encoders write it. + return bytes.Join([][]byte{ftyp, mdat, mp4Box("moov", mp4Box("udta", meta))}, nil) +} + +var lifeForms = []string{ + "Boom Bap", "Downtempo", "Hip Hop", "Instrumental", + "Lo-Fi", "Lo-Fi Hip Hop", "Chillwave", "Instrumental Hip Hop", +} + +func genreFields(values ...string) []string { + out := make([]string, len(values)) + for i, v := range values { + out[i] = "GENRE=" + v + } + return out +} + +// The #2500 regression, shaped like the operator's Kupla - Life Forms files: +// eight GENRE fields, of which dhowden/tag keeps the last. +func TestExtractGenres_FLACRepeatedFields(t *testing.T) { + data := buildFLAC(false, append([]string{"TITLE=Eons"}, genreFields(lifeForms...)...)...) + 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 got := meta.Genre(); got != "Instrumental Hip Hop" { + t.Logf("note: dhowden/tag no longer keeps only the last GENRE (got %q)", got) + } + genres, fellBack := extractGenres(meta, rs) + if fellBack { + t.Error("fellBack = true, want false") + } + if !equalStrings(genres, lifeForms) { + t.Errorf("genres = %q, want %q", genres, lifeForms) + } +} + +func TestReadFLACGenreValues(t *testing.T) { + tests := []struct { + name string + data []byte + want []string + err bool + }{ + {"field names are case-insensitive", buildFLAC(false, "genre=Shoegaze", "Genre=Dream Pop"), []string{"Shoegaze", "Dream Pop"}, false}, + {"behind an ID3v2 tag", buildFLAC(true, "GENRE=Jazz", "GENRE=Bop"), []string{"Jazz", "Bop"}, false}, + {"no genre is not an error", buildFLAC(false, "TITLE=x"), nil, false}, + {"empty values dropped", buildFLAC(false, "GENRE= ", "GENRE=Rock"), []string{"Rock"}, false}, + {"a key that only starts with GENRE", buildFLAC(false, "GENRES=Rock", "GENRE=Jazz"), []string{"Jazz"}, false}, + {"not a FLAC", []byte("RIFF....WAVEfmt "), nil, true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := readFLACGenreValues(bytes.NewReader(tc.data)) + if (err != nil) != tc.err { + t.Fatalf("err = %v, want error: %v", err, tc.err) + } + if !equalStrings(got, tc.want) { + t.Errorf("got %q, want %q", got, tc.want) + } + }) + } +} + +func TestReadFLACGenreValues_TruncatedCommentIsAnError(t *testing.T) { + data := buildFLAC(false, "GENRE=Jazz", "GENRE=Bop") + if _, err := readFLACGenreValues(bytes.NewReader(data[:len(data)-2])); err == nil { + t.Error("err = nil for a cut-off comment block") + } +} + +func TestExtractGenres_OggVorbisRepeatedFields(t *testing.T) { + comment := append([]byte("\x03vorbis"), vorbisComment(genreFields("Shoegaze", "Dream Pop")...)...) + comment = append(comment, 1) // framing bit + rs := bytes.NewReader(buildOgg(7, vorbisIdent(), comment)) + meta, err := tag.ReadFrom(rs) + if err != nil { + t.Fatalf("tag.ReadFrom: %v", err) + } + genres, fellBack := extractGenres(meta, rs) + if fellBack || !equalStrings(genres, []string{"Shoegaze", "Dream Pop"}) { + t.Errorf("genres = %q (fellBack %v), want [Shoegaze Dream Pop]", genres, fellBack) + } +} + +func TestReadOggGenreValues_Opus(t *testing.T) { + head := append([]byte("OpusHead"), make([]byte, 11)...) + tags := append([]byte("OpusTags"), vorbisComment(genreFields("Ambient", "Drone")...)...) + got, err := readOggGenreValues(bytes.NewReader(buildOgg(3, head, tags))) + if err != nil || !equalStrings(got, []string{"Ambient", "Drone"}) { + t.Errorf("got %q, %v; want [Ambient Drone]", got, err) + } +} + +// Cover art makes the comment packet span pages. A reader that stopped at the +// first page boundary would read a truncated block. +func TestReadOggGenreValues_PacketSpansPages(t *testing.T) { + fields := append([]string{"METADATA_BLOCK_PICTURE=" + strings.Repeat("A", 70000)}, genreFields("Jazz", "Bop")...) + comment := append([]byte("\x03vorbis"), vorbisComment(fields...)...) + got, err := readOggGenreValues(bytes.NewReader(buildOgg(9, vorbisIdent(), comment))) + if err != nil || !equalStrings(got, []string{"Jazz", "Bop"}) { + t.Errorf("got %q, %v; want [Jazz Bop]", got, err) + } +} + +func TestReadMP4GenreValues(t *testing.T) { + tests := []struct { + name string + data []byte + want []string + }{ + {"several data atoms in one ©gen", buildMP4(true, mp4Box("\xa9gen", mp4Text("Hip Hop"), mp4Text("Lo-Fi"))), []string{"Hip Hop", "Lo-Fi"}}, + {"repeated ©gen atoms", buildMP4(true, mp4Box("\xa9gen", mp4Text("Hip Hop")), mp4Box("\xa9gen", mp4Text("Lo-Fi"))), []string{"Hip Hop", "Lo-Fi"}}, + {"QuickTime meta without version bytes", buildMP4(false, mp4Box("\xa9gen", mp4Text("Jazz"), mp4Text("Bop"))), []string{"Jazz", "Bop"}}, + {"no ©gen is not an error", buildMP4(true), nil}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := readMP4GenreValues(bytes.NewReader(tc.data)) + if err != nil { + t.Fatalf("err = %v", err) + } + if !equalStrings(got, tc.want) { + t.Errorf("got %q, want %q", got, tc.want) + } + }) + } +} + +func TestExtractGenres_MP4MultiValue(t *testing.T) { + rs := bytes.NewReader(buildMP4(true, mp4Box("\xa9gen", mp4Text("Hip Hop"), mp4Text("Lo-Fi")))) + meta, err := tag.ReadFrom(rs) + if err != nil { + t.Fatalf("tag.ReadFrom: %v", err) + } + genres, fellBack := extractGenres(meta, rs) + if fellBack || !equalStrings(genres, []string{"Hip Hop", "Lo-Fi"}) { + t.Errorf("genres = %q (fellBack %v), want [Hip Hop Lo-Fi]", genres, fellBack) + } +} diff --git a/internal/playlists/seed_missing_db_test.go b/internal/playlists/seed_missing_db_test.go new file mode 100644 index 00000000..25826b09 --- /dev/null +++ b/internal/playlists/seed_missing_db_test.go @@ -0,0 +1,85 @@ +package playlists_test + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +func likeTrack(t *testing.T, pool *pgxpool.Pool, userID, trackID pgtype.UUID) { + t.Helper() + if _, err := pool.Exec(context.Background(), + `INSERT INTO general_likes (user_id, track_id) VALUES ($1, $2)`, + userID, trackID); err != nil { + t.Fatalf("like track: %v", err) + } +} + +func markMissing(t *testing.T, pool *pgxpool.Pool, trackID pgtype.UUID) { + t.Helper() + if _, err := pool.Exec(context.Background(), + `UPDATE tracks SET missing_since = now() WHERE id = $1`, trackID); err != nil { + t.Fatalf("mark missing: %v", err) + } +} + +// TestPickTopPlayedTracksForUser_LikedTierSkipsMissing: with no plays, For +// You seeds from likes. A liked track whose file is gone is not a seed (#2701). +func TestPickTopPlayedTracksForUser_LikedTierSkipsMissing(t *testing.T) { + pool := newPool(t) + u := seedUser(t, pool, "seedmissing") + present := seedTrack(t, pool, "Present", "Seed Artist A") + gone := seedTrack(t, pool, "Gone", "Seed Artist B") + likeTrack(t, pool, u.ID, present.ID) + likeTrack(t, pool, u.ID, gone.ID) + markMissing(t, pool, gone.ID) + + seeds, err := dbq.New(pool).PickTopPlayedTracksForUser(context.Background(), u.ID) + if err != nil { + t.Fatalf("pick seeds: %v", err) + } + if len(seeds) != 1 || seeds[0] != present.ID { + t.Fatalf("seeds = %v, want only the present liked track %v", seeds, present.ID) + } +} + +// TestPickTopPlayedTrackForArtistByUser_FallbackSkipsMissing: with no plays, +// Songs-like seeds from the artist's newest album. When that album's track is +// missing, the seed comes from the next album rather than the missing file. +func TestPickTopPlayedTrackForArtistByUser_FallbackSkipsMissing(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + u := seedUser(t, pool, "songslikemissing") + older := seedTrack(t, pool, "Older", "Songs Like Artist") + newerAlbum := seedAlbumForArtist(t, pool, "Newer Album", older.ArtistID) + newer := seedTrackForArtist(t, pool, "Newer", newerAlbum, older.ArtistID) + if _, err := pool.Exec(ctx, `UPDATE albums SET release_date = '2010-01-01' WHERE id = $1`, older.AlbumID); err != nil { + t.Fatalf("date older album: %v", err) + } + if _, err := pool.Exec(ctx, `UPDATE albums SET release_date = '2020-01-01' WHERE id = $1`, newerAlbum); err != nil { + t.Fatalf("date newer album: %v", err) + } + + q := dbq.New(pool) + args := dbq.PickTopPlayedTrackForArtistByUserParams{UserID: u.ID, ArtistID: older.ArtistID} + got, err := q.PickTopPlayedTrackForArtistByUser(ctx, args) + if err != nil { + t.Fatalf("pick seed: %v", err) + } + if got != newer.ID { + t.Fatalf("before marking: seed = %v, want the newest album's track %v", got, newer.ID) + } + + markMissing(t, pool, newer.ID) + got, err = q.PickTopPlayedTrackForArtistByUser(ctx, args) + if err != nil { + t.Fatalf("pick seed: %v", err) + } + if got != older.ID { + t.Fatalf("after marking: seed = %v, want the present older track %v", got, older.ID) + } +} diff --git a/web/scripts/check-tint-contrast.js b/web/scripts/check-tint-contrast.js new file mode 100644 index 00000000..a86abafc --- /dev/null +++ b/web/scripts/check-tint-contrast.js @@ -0,0 +1,110 @@ +// Finds a hue painted as TEXT on a tint of itself (#3150). The tint sits close +// to the surface under it, so the raw hue on it measures 1.6–2.4:1 against +// AA's 4.5. The fix is the hue's -fg partner (tokens.json colors.fg), mixed +// toward parchment: accent-fg on an accent tint measures 5.03. +// +// Two spellings are checked: +// Tailwind one class string holding both `bg-X-tint` / `bg-X/NN` and `text-X` +// CSS one rule block holding both a `color-mix(… var(--fs-X) N%, +// transparent)` background and `color: var(--fs-X)` +// +// A pair split across two strings (a tint in one class: directive, the text +// in another) is not seen. The lookbehind on `color:` keeps border-color and +// outline-color out: a border is a graphic with a 3:1 floor, not text. +import { readFileSync, readdirSync, statSync } from 'node:fs'; +import { join, relative } from 'node:path'; + +export const HUES = { + accent: 'accent-fg', + warning: 'warning-fg', + error: 'error-fg', + info: 'info-fg', + moss: 'success-fg' +}; + +// The quoted string around index i. Read outward from the tint class rather +// than pairing every quote in the file, which an apostrophe in prose +// ("Couldn't") throws out of step. +function enclosingString(source, i) { + const start = Math.max(...['"', "'", '`'].map((q) => source.lastIndexOf(q, i))); + if (start < 0) return ''; + const end = source.indexOf(source[start], i); + return end < 0 ? '' : source.slice(start, end + 1); +} + +/** Tailwind class strings that put text-X on a bg-X tint. */ +export function findTailwind(source) { + const found = []; + for (const hue of Object.keys(HUES)) { + const tint = new RegExp(`(?<=^|[\\s'"\`])bg-${hue}(?:-tint|/\\d+)(?=$|[\\s'"\`])`, 'g'); + const text = new RegExp(`(?:^|[\\s'"\`])text-${hue}(?=$|[\\s'"\`])`); + for (const m of source.matchAll(tint)) { + if (text.test(enclosingString(source, m.index))) { + found.push({ hue, index: m.index, fix: `text-${hue === 'moss' ? 'success' : hue}-fg` }); + } + } + } + return found; +} + +/** CSS rule blocks that put color: var(--fs-X) on a color-mix tint of X. */ +export function findCss(source) { + const found = []; + for (const m of source.matchAll(/\{([^{}]*)\}/g)) { + for (const hue of Object.keys(HUES)) { + const tint = new RegExp( + `background(?:-color)?\\s*:\\s*color-mix\\(\\s*in srgb\\s*,\\s*var\\(--fs-${hue}\\)\\s*\\d+%\\s*,\\s*transparent\\s*\\)` + ); + const text = new RegExp(`(? { + test('no source paints a hue as text on a tint of itself', () => { + expect(scan(resolve(web, 'src'), web)).toEqual([]); + }); + + describe('Tailwind', () => { + test('finds text-X on bg-X-tint and bg-X/NN in one class string', () => { + expect(findTailwind(`class="rounded bg-accent-tint px-2 text-accent"`)).toHaveLength(1); + expect(findTailwind(`'bg-error/15 text-error'`)).toHaveLength(1); + expect(findTailwind("`bg-moss/10 text-moss`")[0].fix).toBe('text-success-fg'); + }); + + test('a class string spanning lines is one string', () => { + expect(findTailwind(`class="bg-accent-tint\n text-accent"`)).toHaveLength(1); + }); + + test('an apostrophe earlier in the file does not hide a pair', () => { + expect( + findTailwind(`

Couldn't add

\n`) + ).toHaveLength(1); + }); + + test('passes the -fg partner, another hue, and a tint with no hue text', () => { + expect(findTailwind(`"bg-accent-tint text-accent-fg"`)).toEqual([]); + expect(findTailwind(`"bg-accent-tint text-error"`)).toEqual([]); + expect(findTailwind(`"bg-accent-tint text-text-primary"`)).toEqual([]); + expect(findTailwind(`"text-accent"`)).toEqual([]); + }); + }); + + describe('raw error text, on any surface', () => { + test('finds text-error, class:text-error and color: var(--fs-error)', () => { + expect(findRawText(`

x

`)).toHaveLength(1); + expect(findRawText(`
`)).toHaveLength(1); + expect(findRawText(`.msg { color: var(--fs-error); }`)[0].fix).toBe('var(--fs-error-fg)'); + }); + + test('passes error-fg, and error as a border or outline', () => { + expect(findRawText(`

x

`)).toEqual([]); + expect(findRawText(`
`)).toEqual([]); + expect(findRawText(`.a { border-color: var(--fs-error); outline-color: var(--fs-error); }`)).toEqual([]); + }); + }); + + describe('CSS', () => { + const tinted = (color) => + `.pill { background: color-mix(in srgb, var(--fs-warning) 15%, transparent); ${color} }`; + + test('finds color: var(--fs-X) on a color-mix tint of X', () => { + expect(findCss(tinted('color: var(--fs-warning);'))).toHaveLength(1); + expect(findCss(tinted('color: var(--fs-warning);'))[0].fix).toBe('var(--fs-warning-fg)'); + }); + + test('passes the -fg partner and border or outline colours', () => { + expect(findCss(tinted('color: var(--fs-warning-fg);'))).toEqual([]); + expect(findCss(tinted('border-color: var(--fs-warning);'))).toEqual([]); + expect(findCss(tinted('outline-color: var(--fs-warning);'))).toEqual([]); + }); + + test('a tint and a colour in different rules are not a pair', () => { + expect( + findCss( + `.a { background: color-mix(in srgb, var(--fs-accent) 15%, transparent); }\n.b { color: var(--fs-accent); }` + ) + ).toEqual([]); + }); + }); +}); diff --git a/web/scripts/tokens-to-css.js b/web/scripts/tokens-to-css.js index d72f50a3..42495359 100644 --- a/web/scripts/tokens-to-css.js +++ b/web/scripts/tokens-to-css.js @@ -19,6 +19,14 @@ export function emit(tokens) { for (const [name, value] of Object.entries(tokens.colors.flat)) { lines.push(` --fs-${name}: ${value};`); } + // The house -fg tokens (FabledSword, 2026-08-27; #3150): a hue as TEXT on a + // tint of itself, mixed toward parchment. Parchment inverts in light mode, + // so one declaration lightens the text on dark surfaces and darkens it on + // light ones. Declared on :root, the element data-theme is set on, so the + // var()s resolve against that mode's parchment. + for (const [name, value] of Object.entries(tokens.colors.fg ?? {})) { + lines.push(` --fs-${name}: ${value};`); + } for (const [name, value] of Object.entries(tokens.radii)) { lines.push(` --fs-radius-${name}: ${value};`); } diff --git a/web/scripts/tokens-to-css.test.js b/web/scripts/tokens-to-css.test.js index 0190d81c..2b38a578 100644 --- a/web/scripts/tokens-to-css.test.js +++ b/web/scripts/tokens-to-css.test.js @@ -5,7 +5,8 @@ const sample = { colors: { dark: { obsidian: '#000', parchment: '#FFF' }, light: { obsidian: '#FFF', parchment: '#000' }, - flat: { accent: '#4A6B5C', 'on-action': '#E8E4D8' } + flat: { accent: '#4A6B5C', 'on-action': '#E8E4D8' }, + fg: { 'accent-fg': 'color-mix(in srgb, var(--fs-accent) 45%, var(--fs-parchment))' } }, radii: { sm: '4px' }, fontStacks: { @@ -40,4 +41,13 @@ describe('tokens-to-css emit()', () => { expect(css).toContain('--fs-radius-sm: 4px;'); expect(css).toContain("--fs-font-display: 'Fraunces', serif;"); }); + + test('emits the -fg tokens in :root, not the light block', () => { + const css = emit(sample); + const root = css.match(/:root \{([\s\S]*?)\}/); + expect(root).not.toBeNull(); + expect(root[1]).toContain('--fs-accent-fg: color-mix(in srgb, var(--fs-accent) 45%, var(--fs-parchment));'); + const light = css.match(/\[data-theme="light"\] \{([\s\S]*?)\}/); + expect(light[1]).not.toContain('-fg'); + }); }); diff --git a/web/src/lib/components/DiscoverResultCard.svelte b/web/src/lib/components/DiscoverResultCard.svelte index ea3ae967..a4710371 100644 --- a/web/src/lib/components/DiscoverResultCard.svelte +++ b/web/src/lib/components/DiscoverResultCard.svelte @@ -169,7 +169,7 @@ font-size: 11px; line-height: 14px; background: color-mix(in srgb, var(--fs-accent) 15%, transparent); - color: var(--fs-accent); + color: var(--fs-accent-fg); } /* Muted rather than accented: a parked card should recede, not compete with the live suggestions around it. Same geometry as .kept-pill so the diff --git a/web/src/lib/components/FlagPopover.svelte b/web/src/lib/components/FlagPopover.svelte index a3a75db6..ebccc63a 100644 --- a/web/src/lib/components/FlagPopover.svelte +++ b/web/src/lib/components/FlagPopover.svelte @@ -79,7 +79,7 @@ > {#if error} -

Couldn't save flag — {error}

+

Couldn't save flag — {error}

{/if}
diff --git a/web/src/lib/styles/tokens.generated.css b/web/src/lib/styles/tokens.generated.css index 0ebfd72f..af3a9d32 100644 --- a/web/src/lib/styles/tokens.generated.css +++ b/web/src/lib/styles/tokens.generated.css @@ -15,6 +15,11 @@ --fs-info: #3D5A6E; --fs-accent: #4A6B5C; --fs-on-action: #E8E4D8; + --fs-accent-fg: color-mix(in srgb, var(--fs-accent) 45%, var(--fs-parchment)); + --fs-success-fg: color-mix(in srgb, var(--fs-moss) 45%, var(--fs-parchment)); + --fs-warning-fg: color-mix(in srgb, var(--fs-warning) 50%, var(--fs-parchment)); + --fs-error-fg: color-mix(in srgb, var(--fs-error) 50%, var(--fs-parchment)); + --fs-info-fg: color-mix(in srgb, var(--fs-info) 50%, var(--fs-parchment)); --fs-radius-sm: 4px; --fs-radius-md: 8px; --fs-radius-lg: 12px; diff --git a/web/src/lib/styles/tokens.json b/web/src/lib/styles/tokens.json index c602dfe7..91776147 100644 --- a/web/src/lib/styles/tokens.json +++ b/web/src/lib/styles/tokens.json @@ -27,6 +27,13 @@ "info": "#3D5A6E", "accent": "#4A6B5C", "on-action": "#E8E4D8" + }, + "fg": { + "accent-fg": "color-mix(in srgb, var(--fs-accent) 45%, var(--fs-parchment))", + "success-fg": "color-mix(in srgb, var(--fs-moss) 45%, var(--fs-parchment))", + "warning-fg": "color-mix(in srgb, var(--fs-warning) 50%, var(--fs-parchment))", + "error-fg": "color-mix(in srgb, var(--fs-error) 50%, var(--fs-parchment))", + "info-fg": "color-mix(in srgb, var(--fs-info) 50%, var(--fs-parchment))" } }, "radii": { "sm": "4px", "md": "8px", "lg": "12px", "xl": "16px" }, diff --git a/web/src/routes/admin/diagnostics/+page.svelte b/web/src/routes/admin/diagnostics/+page.svelte index 7224421b..bd7c487f 100644 --- a/web/src/routes/admin/diagnostics/+page.svelte +++ b/web/src/routes/admin/diagnostics/+page.svelte @@ -325,7 +325,7 @@ {#if diagQuery.isError} -

Couldn't load: {errMessage(diagQuery.error)}

+

Couldn't load: {errMessage(diagQuery.error)}

{:else if diagQuery.isPending}

Loading…

{:else if rows.length === 0} diff --git a/web/src/routes/admin/duplicates/+page.svelte b/web/src/routes/admin/duplicates/+page.svelte index fb27e7d3..5cf7a768 100644 --- a/web/src/routes/admin/duplicates/+page.svelte +++ b/web/src/routes/admin/duplicates/+page.svelte @@ -138,7 +138,7 @@

Duplicates

{#if total > 0} {total} @@ -188,7 +188,7 @@ {#if query.isPending}

Loading duplicates…

{:else if query.isError} -

Couldn't load the duplicates report.

+

Couldn't load the duplicates report.

{:else if groups.length === 0} @@ -302,7 +302,7 @@ diff --git a/web/src/routes/admin/integrations/+page.svelte b/web/src/routes/admin/integrations/+page.svelte index 9d6bae2d..f98096b1 100644 --- a/web/src/routes/admin/integrations/+page.svelte +++ b/web/src/routes/admin/integrations/+page.svelte @@ -463,7 +463,7 @@ {/each} {#if testListErrors.quality_profiles} -

+

Couldn't fetch quality profiles — {testListErrors.quality_profiles}

{/if} @@ -481,7 +481,7 @@ {/each} {#if testListErrors.metadata_profiles} -

+

Couldn't fetch metadata profiles — {testListErrors.metadata_profiles}

{/if} @@ -501,7 +501,7 @@ {/each} {#if testListErrors.root_folders} -

+

Couldn't fetch root folders — {testListErrors.root_folders}

{/if} @@ -513,11 +513,11 @@ Connected — Lidarr {testResult.version}

{:else} -

Connection failed — {testResult.error}

+

Connection failed — {testResult.error}

{/if} {/if} {#if saveError} -

Save failed — {saveError}

+

Save failed — {saveError}

{/if}
@@ -625,7 +625,7 @@ OK{coverTestResults[provider.id].duration_ms ? ` (${coverTestResults[provider.id].duration_ms}ms)` : ''}

{:else} -

+

Failed — {coverTestResults[provider.id].error}

{/if} @@ -722,7 +722,7 @@ OK{tagTestResults[provider.id].duration_ms ? ` (${tagTestResults[provider.id].duration_ms}ms)` : ''}

{:else} -

+

Failed — {tagTestResults[provider.id].error}

{/if} @@ -837,7 +837,7 @@ class="mt-3 w-full rounded-md border border-border bg-background px-3 py-2 font-mono text-sm text-text-primary placeholder:text-text-muted focus:outline-none focus:ring-2 focus:ring-accent" /> {#if disconnectError} -

Disconnect failed — {disconnectError}

+

Disconnect failed — {disconnectError}

{/if}