From 37d3a5bcd395fb6a48644a0c4032614ef2764607 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 7 Oct 2026 23:40:56 -0400 Subject: [PATCH] fix(library): read every genre of FLAC, Ogg, Opus and MP4 files (#2500) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Vorbis comments repeat a field to give it several values (GENRE=Boom Bap, GENRE=Downtempo, ...). dhowden/tag keeps comments in a map keyed by field name, so each repeat overwrote the previous one and only the last genre was stored. MP4 has the same gap: several data atoms in one ©gen atom, or repeated ©gen atoms, collapse to one value. On the operator's library 3,658 of 4,092 FLACs declare more than one genre. Kupla's Life Forms carries eight and was stored as "Instrumental Hip Hop" alone, so browse and the taste profile never saw the other seven. - vorbisgenre.go reads the comment block directly: FLAC's metadata block (including FLACs behind an ID3v2 tag), and the comment packet of Ogg Vorbis and Opus, reassembled across pages when cover art makes it span several. - mp4genre.go walks moov > udta > meta > ilst and returns every ©gen text value. It handles ISO and QuickTime meta layouts and a moov placed after mdat. A file with only the numeric gnre atom still falls back to dhowden, which resolves it. - extractGenres routes VORBIS and MP4 through them, as #2499 did for ID3v2. - tagReadVersion 3 -> 4, so the next scan re-reads the tags of files already indexed. Unchanged files keep their duration and fingerprint, so the pass costs tag reads only. Co-Authored-By: Claude Opus 5.5 --- internal/library/genre.go | 24 ++- internal/library/mp4genre.go | 168 ++++++++++++++++ internal/library/scanner.go | 6 +- internal/library/vorbisgenre.go | 214 ++++++++++++++++++++ internal/library/vorbisgenre_test.go | 288 +++++++++++++++++++++++++++ 5 files changed, 694 insertions(+), 6 deletions(-) create mode 100644 internal/library/mp4genre.go create mode 100644 internal/library/vorbisgenre.go create mode 100644 internal/library/vorbisgenre_test.go 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) + } +}