diff --git a/CLAUDE.md b/CLAUDE.md index 97eb7a5..5628c6f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2549,11 +2549,38 @@ Five things about it are load-bearing, and four of them fail silently: correctly. Confidently wrong is worse than absent here, which is the same rule `Known` exists for. -One gap this did not close, and it is older: **`dhowden/tag` has no -RIFF reader**, so nothing the tag writer puts in a WAV's `id3 ` chunk -is visible to `metadata.ExtractTags` — not the totals and not the title -either. `wav_test.go` reads that chunk itself, which is why no test -ever noticed. +One gap this did not close and #104 did: **`dhowden/tag` has no RIFF +reader**, so nothing the tag writer put in a WAV's `id3 ` chunk was +visible to `metadata.ExtractTags` — not the totals and not the title +either, on files the app itself had just tagged. `wav_test.go` read +that chunk itself, which is why no test noticed: a round trip asserted +through the writer's own parser is a test of the writer. + +`backend/riff` is where the container is now read, and it is its own +package because the alternative is an import cycle — `tagwriter` +imports `metadata`, so `metadata` cannot reach back for `parseRIFF`. +`backend/tagtotals` is the precedent. + +Three things about it are load-bearing. **The two readers are +deliberately different**: `Parse` holds every chunk in memory, which is +what rewriting a file needs, and a WAV's audio *is* a chunk — so the +scan path uses `ID3Chunk`, which seeks over what it is not looking for. +**The container decides, before `tag.ReadFrom` rather than after it +fails**, because that library's last resort is an ID3v1 trailer and a +WAV carrying both would otherwise be read by the wrong one. And **an +untagged WAV is a file with no tags, not a file with a problem**: no +chunk, an RF64 container or a tag holding no frames all read as empty +metadata with no `TagReadWarning`, since the scanner's filename +fallback is the right answer and a warning would put a fault on a file +that has none. + +The gap was pinned by a test that said so, which failed the moment the +reader learned and carried the instructions for what to update in its +own comment. So it is deleted, `TestFixturesMatchManifest` no longer +skips `wav`, and `totals_test.go`'s WAV case goes through +`metadata.ExtractTags` like the other three formats. The fixture +library's two WAV tracks scan with their tags and their cover now, +which is a change to what every seeded tier sees. **The absence is what gets marked, not the presence.** The tracklist put a green tick against every owned track and a legend underneath diff --git a/backend/metadata/tags.go b/backend/metadata/tags.go index c734848..ed3e68d 100644 --- a/backend/metadata/tags.go +++ b/backend/metadata/tags.go @@ -74,6 +74,14 @@ func ExtractTags(path string) (*TrackMetadata, error) { // ExtractTagsFromReader reads metadata from an io.ReadSeeker. func ExtractTagsFromReader(r io.ReadSeeker) (*TrackMetadata, error) { + // The container decides, so this is asked before tag.ReadFrom and + // not after its failure: a WAV's tags live in a RIFF chunk that + // dhowden/tag cannot see, and its fallback -- an ID3v1 trailer -- + // would otherwise outrank them. + if meta, ok := wavTags(r); ok { + return meta, nil + } + m, err := tag.ReadFrom(r) if err != nil { // No tags found is not necessarily an error - return empty metadata diff --git a/backend/metadata/tags_wav.go b/backend/metadata/tags_wav.go new file mode 100644 index 0000000..56767f5 --- /dev/null +++ b/backend/metadata/tags_wav.go @@ -0,0 +1,68 @@ +package metadata + +import ( + "bytes" + "errors" + "fmt" + "io" + "strings" + + "yellowjacket/backend/riff" +) + +// wavTags reads the ID3v2 tag a WAV carries in its RIFF "id3 " chunk, +// which is where backend/tagwriter puts it and where dhowden/tag -- +// having no RIFF reader at all -- cannot look. Without this a WAV +// scans as an untagged file however carefully it was tagged. +// +// ok is false when r is not a RIFF/WAVE container, and the read +// position is restored either way so the caller can carry on. +func wavTags(r io.ReadSeeker) (*TrackMetadata, bool) { + start, err := r.Seek(0, io.SeekCurrent) + if err != nil { + return nil, false + } + + id3Data, chunkErr := riff.ID3Chunk(r) + + if _, err := r.Seek(start, io.SeekStart); err != nil { + return nil, false + } + + switch { + case chunkErr == nil: + return wavTagsFrom(id3Data), true + + // Not ours to read: let the ordinary dispatch have the file. + case errors.Is(chunkErr, riff.ErrNotRIFF), errors.Is(chunkErr, riff.ErrNotWAVE): + return nil, false + + // A RIFF container we cannot get a tag out of -- no chunk, an RF64 + // file, a truncated header. That is a file with no readable tags, + // which is what the scanner's filename fallback is for. + default: + return &TrackMetadata{}, true + } +} + +// wavTagsFrom parses the bytes of a WAV's ID3v2 chunk. +func wavTagsFrom(id3Data []byte) *TrackMetadata { + meta, err := extractID3v2Lenient(bytes.NewReader(id3Data)) + if err != nil { + // A tag holding no frames is not a damaged tag: writing every + // field back out empty leaves one, and warning about it would + // put a fault on a file that has none. + if errors.Is(err, ErrTagsUnreadable) { + return &TrackMetadata{} + } + + return &TrackMetadata{ + TagReadWarning: fmt.Errorf("%w: %w", ErrTagsUnreadable, err), + } + } + + // extractID3v2Lenient names MP3, being the recovery path for one. + meta.FileFormat = strings.ToUpper(strings.TrimPrefix(string(WAV), ".")) + + return meta +} diff --git a/backend/riff/riff.go b/backend/riff/riff.go new file mode 100644 index 0000000..e60d265 --- /dev/null +++ b/backend/riff/riff.go @@ -0,0 +1,183 @@ +// Package riff reads the chunk layout of a RIFF/WAVE container. +// +// It exists because both halves of WAV tagging need it and neither can +// import the other: backend/tagwriter writes a WAV's tags into a RIFF +// "id3 " chunk and already imports backend/metadata, which is what has +// to read them back out. backend/tagtotals is the precedent. +// +// The two readers here are deliberately different. Parse holds every +// chunk's data in memory, which is what rewriting a file needs; a WAV's +// audio *is* the "data" chunk, so doing that on the scan path would +// read every library file in full. ID3Chunk seeks over what it is not +// looking for instead. Both walk the same headers. +package riff + +import ( + "bytes" + "encoding/binary" + "errors" + "fmt" + "io" + "strings" +) + +// Sentinel errors describing a container this package will not read. +var ( + ErrRF64NotSupported = errors.New("RF64 files are not yet supported") + ErrNotRIFF = errors.New("not a RIFF file") + ErrNotWAVE = errors.New("not a WAVE file") + ErrNoID3Chunk = errors.New("no ID3 chunk in RIFF file") +) + +// Chunk holds a single RIFF sub-chunk (ID + raw data). +type Chunk struct { + ID [4]byte + Data []byte +} + +// IsID3 reports whether id is that of an ID3v2 RIFF chunk. Both +// lowercase "id3 " and uppercase "ID3 " are accepted. +func IsID3(id [4]byte) bool { + return strings.ToLower(string(id[:3])) == "id3" +} + +// Parse reads every RIFF sub-chunk from r, in order, starting at the +// reader's current position. It rejects RF64 files and non-WAVE +// containers with descriptive errors. The parser is lenient: it +// tolerates a missing final padding byte and ignores the declared +// RIFF size. +func Parse(r io.Reader) ([]Chunk, error) { + if err := readContainer(r); err != nil { + return nil, err + } + + var chunks []Chunk + + for { + id, size, err := nextHeader(r) + if errors.Is(err, io.EOF) { + break + } + + if err != nil { + return nil, err + } + + data := make([]byte, size) + if _, err := io.ReadFull(r, data); err != nil { + return nil, fmt.Errorf("read chunk data for %q: %w", id, err) + } + + chunks = append(chunks, Chunk{ID: id, Data: data}) + + // Odd-length chunks have a padding byte. Lenient: if the + // read fails (e.g. EOF), just break rather than error. + if size%2 != 0 { + var pad [1]byte + + if _, err := r.Read(pad[:]); err != nil { + break + } + } + } + + return chunks, nil +} + +// ID3Chunk returns the payload of the ID3v2 chunk of the RIFF/WAVE +// container at the reader's current position, seeking over every other +// chunk rather than reading it. It returns ErrNoID3Chunk when the +// container carries no such chunk, and leaves the read position +// unspecified either way. +func ID3Chunk(r io.ReadSeeker) ([]byte, error) { + if err := readContainer(r); err != nil { + return nil, err + } + + for { + id, size, err := nextHeader(r) + if errors.Is(err, io.EOF) { + return nil, ErrNoID3Chunk + } + + if err != nil { + return nil, err + } + + if !IsID3(id) { + // Odd-length chunks carry a padding byte. Seeking past + // the end of the file is not an error; the next header + // read is what reports the end. + if _, err := r.Seek(int64(size)+int64(size%2), io.SeekCurrent); err != nil { + return nil, fmt.Errorf("skip chunk %q: %w", id, err) + } + + continue + } + + // Copied rather than allocated up front: a truncated file is + // free to declare a chunk larger than the whole of itself. + var data bytes.Buffer + if _, err := io.CopyN(&data, r, int64(size)); err != nil { + return nil, fmt.Errorf("read chunk data for %q: %w", id, err) + } + + return data.Bytes(), nil + } +} + +// readContainer consumes the 12-byte RIFF/WAVE header at the reader's +// current position. +func readContainer(r io.Reader) error { + var magic [4]byte + if _, err := io.ReadFull(r, magic[:]); err != nil { + return fmt.Errorf("read RIFF magic: %w", err) + } + + if string(magic[:]) == "RF64" { + return ErrRF64NotSupported + } + + if string(magic[:]) != "RIFF" { + return fmt.Errorf("%w: got %q", ErrNotRIFF, magic) + } + + // Read (and discard) RIFF size — lenient, do not enforce. + var riffSize uint32 + if err := binary.Read(r, binary.LittleEndian, &riffSize); err != nil { + return fmt.Errorf("read RIFF size: %w", err) + } + + var form [4]byte + if _, err := io.ReadFull(r, form[:]); err != nil { + return fmt.Errorf("read WAVE form type: %w", err) + } + + if string(form[:]) != "WAVE" { + return fmt.Errorf("%w: got %q", ErrNotWAVE, form) + } + + return nil +} + +// nextHeader reads one sub-chunk header. It returns io.EOF once the +// chunks are exhausted, including for a header cut short. +func nextHeader(r io.Reader) ([4]byte, uint32, error) { + var id [4]byte + + _, err := io.ReadFull(r, id[:]) + if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) { + return id, 0, io.EOF + } + + if err != nil { + return id, 0, fmt.Errorf("read chunk ID: %w", err) + } + + var size uint32 + if err := binary.Read(r, binary.LittleEndian, &size); err != nil { + return id, 0, fmt.Errorf("read chunk size for %q: %w", id, err) + } + + return id, size, nil +} diff --git a/backend/riff/riff_test.go b/backend/riff/riff_test.go new file mode 100644 index 0000000..19295ff --- /dev/null +++ b/backend/riff/riff_test.go @@ -0,0 +1,211 @@ +package riff_test + +import ( + "bytes" + "encoding/binary" + "errors" + "testing" + + "yellowjacket/backend/riff" +) + +// chunk is one sub-chunk to put in a test container. +type chunk struct { + id string + data []byte +} + +// buildRIFF assembles a container from magic, form type and chunks, +// padding odd-length chunks the way a writer must. +func buildRIFF(magic, form string, chunks []chunk) []byte { + var body bytes.Buffer + + body.WriteString(form) + + for _, c := range chunks { + body.WriteString(c.id) + _ = binary.Write(&body, binary.LittleEndian, uint32(len(c.data))) + body.Write(c.data) + + if len(c.data)%2 != 0 { + body.WriteByte(0) + } + } + + var out bytes.Buffer + + out.WriteString(magic) + _ = binary.Write(&out, binary.LittleEndian, uint32(body.Len())) + out.Write(body.Bytes()) + + return out.Bytes() +} + +func TestID3Chunk_FindsTheTagPastTheAudio(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + chunks []chunk + want string + }{ + { + name: "after an odd-length chunk", + chunks: []chunk{ + {id: "fmt ", data: make([]byte, 16)}, + {id: "LIST", data: []byte("INFOodd")}, + {id: "data", data: make([]byte, 200)}, + {id: "id3 ", data: []byte("ID3vTAG")}, + }, + want: "ID3vTAG", + }, + { + // The chunk ID is written both ways in the wild, and the + // writer accepts either, so the reader must too. + name: "uppercase ID3", + chunks: []chunk{ + {id: "data", data: make([]byte, 8)}, + {id: "ID3 ", data: []byte("upper")}, + }, + want: "upper", + }, + { + name: "first chunk", + chunks: []chunk{ + {id: "id3 ", data: []byte("first")}, + {id: "data", data: make([]byte, 8)}, + }, + want: "first", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + r := bytes.NewReader(buildRIFF("RIFF", "WAVE", tc.chunks)) + + got, err := riff.ID3Chunk(r) + if err != nil { + t.Fatalf("ID3Chunk: %v", err) + } + + if string(got) != tc.want { + t.Errorf("chunk data: got %q, want %q", got, tc.want) + } + }) + } +} + +func TestID3Chunk_RejectsWhatItCannotRead(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + bytes []byte + want error + }{ + { + name: "no ID3 chunk", + bytes: buildRIFF("RIFF", "WAVE", []chunk{{id: "data", data: []byte{1, 2}}}), + want: riff.ErrNoID3Chunk, + }, + { + name: "no chunks at all", + bytes: buildRIFF("RIFF", "WAVE", nil), + want: riff.ErrNoID3Chunk, + }, + { + name: "not RIFF", + bytes: []byte("ID3\x03\x00\x00\x00\x00\x00\x00\x00\x00"), + want: riff.ErrNotRIFF, + }, + { + name: "not WAVE", + bytes: buildRIFF("RIFF", "AVI ", []chunk{{id: "id3 ", data: []byte("x")}}), + want: riff.ErrNotWAVE, + }, + { + name: "RF64", + bytes: buildRIFF("RF64", "WAVE", []chunk{{id: "id3 ", data: []byte("x")}}), + want: riff.ErrRF64NotSupported, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + _, err := riff.ID3Chunk(bytes.NewReader(tc.bytes)) + if !errors.Is(err, tc.want) { + t.Errorf("ID3Chunk error: got %v, want %v", err, tc.want) + } + }) + } +} + +// A file cut short mid-chunk is a file with no tag, not a reason to +// allocate the size it claims: the declared size is four bytes any +// truncation can leave saying 4 GB. +func TestID3Chunk_ToleratesATruncatedFile(t *testing.T) { + t.Parallel() + + full := buildRIFF("RIFF", "WAVE", []chunk{ + {id: "data", data: make([]byte, 64)}, + {id: "id3 ", data: []byte("tag")}, + }) + + t.Run("cut inside the audio", func(t *testing.T) { + t.Parallel() + + _, err := riff.ID3Chunk(bytes.NewReader(full[:32])) + if !errors.Is(err, riff.ErrNoID3Chunk) { + t.Errorf("ID3Chunk error: got %v, want %v", err, riff.ErrNoID3Chunk) + } + }) + + t.Run("cut inside the tag", func(t *testing.T) { + t.Parallel() + + if _, err := riff.ID3Chunk(bytes.NewReader(full[:len(full)-2])); err == nil { + t.Error("ID3Chunk: got nil error for a truncated tag chunk") + } + }) +} + +// Parse is the writer's half and reads every chunk into memory, which +// is what preserving them needs. +func TestParse_ReadsEveryChunkInOrder(t *testing.T) { + t.Parallel() + + raw := buildRIFF("RIFF", "WAVE", []chunk{ + {id: "fmt ", data: make([]byte, 16)}, + {id: "LIST", data: []byte("INFOodd")}, + {id: "id3 ", data: []byte("tag")}, + }) + + chunks, err := riff.Parse(bytes.NewReader(raw)) + if err != nil { + t.Fatalf("Parse: %v", err) + } + + want := []string{"fmt ", "LIST", "id3 "} + if len(chunks) != len(want) { + t.Fatalf("chunk count: got %d, want %d", len(chunks), len(want)) + } + + for i, id := range want { + if got := string(chunks[i].ID[:]); got != id { + t.Errorf("chunk %d: got %q, want %q", i, got, id) + } + } + + if !riff.IsID3(chunks[2].ID) || string(chunks[2].Data) != "tag" { + t.Errorf("id3 chunk: got %q", chunks[2].Data) + } + + // The padding byte after an odd chunk is not part of its data. + if string(chunks[1].Data) != "INFOodd" { + t.Errorf("odd chunk data: got %q, want %q", chunks[1].Data, "INFOodd") + } +} diff --git a/backend/tagwriter/totals_test.go b/backend/tagwriter/totals_test.go index 860b4ea..c40b993 100644 --- a/backend/tagwriter/totals_test.go +++ b/backend/tagwriter/totals_test.go @@ -13,9 +13,10 @@ import ( // indistinguishable from never having written one. So these assert the // round trip through the *reader the scan uses*, not the bytes. // -// WAV is the exception and it is not this change's: dhowden/tag has no -// RIFF reader at all, so metadata.ExtractTags cannot see a WAV's ID3 -// chunk -- which is why every other test here reads that chunk itself. +// WAV was the exception until #104 -- dhowden/tag has no RIFF reader, +// so metadata.ExtractTags could not see a WAV's ID3 chunk and this +// case read the chunk itself, which is a test of the writer wearing +// the shape of a round trip. All four go through the scanner now. func TestWriteTotals_RoundTripsInEveryFormat(t *testing.T) { t.Parallel() @@ -91,7 +92,7 @@ func TestWriteTotals_RoundTripsInEveryFormat(t *testing.T) { }, { name: "wav", - read: readWavID3Tags, + read: viaScanner, write: func(t *testing.T, dir string) string { t.Helper() diff --git a/backend/tagwriter/wav.go b/backend/tagwriter/wav.go index 583324f..215bd15 100644 --- a/backend/tagwriter/wav.go +++ b/backend/tagwriter/wav.go @@ -8,116 +8,22 @@ import ( "io" "log/slog" "os" - "strings" id3v2 "github.com/bogem/id3v2/v2" "yellowjacket/backend/fileutil" + "yellowjacket/backend/riff" ) -// Sentinel errors for WAV RIFF operations. -var ( - errRF64NotSupported = errors.New("RF64 files are not yet supported") - errNotRIFF = errors.New("not a RIFF file") - errNotWAVE = errors.New("not a WAVE file") - errFileTooLargeForWAV = errors.New("file too large for WAV format (>4GB)") -) - -// riffChunk holds a single RIFF sub-chunk (ID + raw data). -type riffChunk struct { - id [4]byte - data []byte -} - -// parseRIFF reads all RIFF sub-chunks from r. It rejects RF64 files -// and non-WAVE containers with descriptive errors. The parser is -// lenient on read: it tolerates missing padding bytes and ignores -// the declared RIFF size. -func parseRIFF(r io.ReadSeeker) ([]riffChunk, error) { - // Read 4-byte container magic. - var magic [4]byte - if _, err := io.ReadFull(r, magic[:]); err != nil { - return nil, fmt.Errorf("read RIFF magic: %w", err) - } - - if string(magic[:]) == "RF64" { - return nil, errRF64NotSupported - } - - if string(magic[:]) != "RIFF" { - return nil, fmt.Errorf("%w: got %q", errNotRIFF, magic) - } - - // Read (and discard) RIFF size — lenient, do not enforce. - var riffSize uint32 - if err := binary.Read(r, binary.LittleEndian, &riffSize); err != nil { - return nil, fmt.Errorf("read RIFF size: %w", err) - } - - // Read 4-byte form type. - var form [4]byte - if _, err := io.ReadFull(r, form[:]); err != nil { - return nil, fmt.Errorf("read WAVE form type: %w", err) - } - - if string(form[:]) != "WAVE" { - return nil, fmt.Errorf("%w: got %q", errNotWAVE, form) - } - - // Read sub-chunks until EOF. - var chunks []riffChunk - - for { - var chunkID [4]byte - - _, err := io.ReadFull(r, chunkID[:]) - if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) { - break - } - - if err != nil { - return nil, fmt.Errorf("read chunk ID: %w", err) - } - - var chunkSize uint32 - if err := binary.Read(r, binary.LittleEndian, &chunkSize); err != nil { - return nil, fmt.Errorf("read chunk size for %q: %w", chunkID, err) - } - - data := make([]byte, chunkSize) - if _, err := io.ReadFull(r, data); err != nil { - return nil, fmt.Errorf("read chunk data for %q: %w", chunkID, err) - } - - chunks = append(chunks, riffChunk{id: chunkID, data: data}) - - // Odd-length chunks have a padding byte. Lenient: if the - // read fails (e.g. EOF), just break rather than error. - if chunkSize%2 != 0 { - var pad [1]byte - - if _, err := r.Read(pad[:]); err != nil { - break - } - } - } - - return chunks, nil -} - -// isID3ChunkID returns true if id represents an ID3v2 RIFF chunk. -// Both lowercase "id3 " and uppercase "ID3 " are accepted. -func isID3ChunkID(id [4]byte) bool { - s := strings.ToLower(string(id[:3])) - - return s == "id3" -} +// errFileTooLargeForWAV is the one RIFF error that belongs to the +// writer; reading rejects a container in backend/riff. +var errFileTooLargeForWAV = errors.New("file too large for WAV format (>4GB)") // writeRIFF writes a complete RIFF/WAVE container to w, preserving // the given chunks in order and appending the id3Data as the final // "id3 " chunk. Returns errFileTooLargeForWAV if the result would // exceed the 4 GB RIFF limit. -func writeRIFF(w io.Writer, chunks []riffChunk, id3Data []byte) error { +func writeRIFF(w io.Writer, chunks []riff.Chunk, id3Data []byte) error { // Calculate total RIFF payload size: // 4 bytes (WAVE form type) // + for each preserved chunk: 8 (header) + len(data) + padding @@ -125,7 +31,7 @@ func writeRIFF(w io.Writer, chunks []riffChunk, id3Data []byte) error { riffPayload := uint64(4) for _, c := range chunks { - sz := uint64(len(c.data)) + sz := uint64(len(c.Data)) riffPayload += 8 + sz if sz%2 != 0 { @@ -162,7 +68,7 @@ func writeRIFF(w io.Writer, chunks []riffChunk, id3Data []byte) error { // Write each preserved chunk. for _, c := range chunks { - if err := writeChunk(w, c.id, c.data); err != nil { + if err := writeChunk(w, c.ID, c.Data); err != nil { return err } } @@ -225,7 +131,7 @@ func writeWavTags( return fmt.Errorf("open wav for reading: %w", err) } - allChunks, err := parseRIFF(f) + allChunks, err := riff.Parse(f) // Close immediately — we need the handle released before // AtomicWrite creates the replacement file. @@ -237,13 +143,13 @@ func writeWavTags( // Separate preserved chunks from existing ID3 data. var ( - preserved []riffChunk + preserved []riff.Chunk existingID3 []byte ) for _, c := range allChunks { - if isID3ChunkID(c.id) { - existingID3 = c.data + if riff.IsID3(c.ID) { + existingID3 = c.Data } else { preserved = append(preserved, c) } diff --git a/backend/tagwriter/wav_test.go b/backend/tagwriter/wav_test.go index e8fa853..5a57cba 100644 --- a/backend/tagwriter/wav_test.go +++ b/backend/tagwriter/wav_test.go @@ -12,6 +12,7 @@ import ( id3v2 "github.com/bogem/id3v2/v2" "yellowjacket/backend/metadata" + "yellowjacket/backend/riff" ) // createTestWAV builds a minimal valid WAV file with an optional @@ -270,6 +271,88 @@ func TestWriteWavTags_PartialUpdate(t *testing.T) { assertStrField(t, "Composer", meta.Composer, "Original Composer") } +// The writer has always been correct and the reader could not see it: +// a WAV tagged by this app scanned as an untagged file, so editing +// tags, autotagging a folder or importing a WAV download all appeared +// to work and changed nothing the library could show (#104). So this +// asserts the write through metadata.ExtractTags -- the reader the +// scan uses -- rather than through the id3 chunk. +func TestWriteWavTags_ReadBackByTheScanner(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + path := createTestWAV(t, dir, "scanner.wav", nil) + art := tinyJPEG(t) + + changes := TagChanges{ + FieldTitle: "Some Song", + FieldArtist: "Some Artist", + FieldAlbum: "Some Album", + FieldAlbumArtist: "Some Album Artist", + FieldGenre: "Rock", + FieldYear: 2024, + FieldTrackNumber: 3, + FieldComposer: "Some Composer", + FieldCoverArt: art, + } + + if err := writeWavTags(testLogger(), path, changes); err != nil { + t.Fatalf("writeWavTags: %v", err) + } + + meta, err := metadata.ExtractTags(path) + if err != nil { + t.Fatalf("ExtractTags: %v", err) + } + + if meta.TagReadWarning != nil { + t.Errorf("TagReadWarning: %v", meta.TagReadWarning) + } + + assertStrField(t, "Title", meta.Title, "Some Song") + assertStrField(t, "Artist", meta.Artist, "Some Artist") + assertStrField(t, "Album", meta.Album, "Some Album") + assertStrField(t, "AlbumArtist", meta.AlbumArtist, "Some Album Artist") + assertStrField(t, "Genre", meta.Genre, "Rock") + assertStrField(t, "Composer", meta.Composer, "Some Composer") + assertStrField(t, "FileFormat", meta.FileFormat, "WAV") + assertIntField(t, "Year", meta.Year, 2024) + assertIntField(t, "TrackNumber", meta.TrackNumber, 3) + + if !strings.HasPrefix(meta.TagFormat, "ID3v2") { + t.Errorf("TagFormat: got %q, want an ID3v2 version", meta.TagFormat) + } + + if meta.Picture == nil { + t.Fatal("expected cover art, got nil") + } + + if !bytes.Equal(meta.Picture.Data, art) { + t.Errorf("picture data mismatch: got %d bytes, want %d", + len(meta.Picture.Data), len(art)) + } +} + +// An untagged WAV is a file with no tags, not a file with a problem: +// the scanner falls back to the filename and must not be handed a +// warning to surface about it. +func TestUntaggedWav_ReadsAsEmptyWithoutAWarning(t *testing.T) { + t.Parallel() + + path := createTestWAV(t, t.TempDir(), "bare.wav", nil) + + meta, err := metadata.ExtractTags(path) + if err != nil { + t.Fatalf("ExtractTags: %v", err) + } + + if meta.TagReadWarning != nil { + t.Errorf("TagReadWarning: %v", meta.TagReadWarning) + } + + assertStrField(t, "Title", meta.Title, "") +} + func TestWriteWavTags_ChunkPreservation(t *testing.T) { t.Parallel() @@ -282,7 +365,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) { t.Fatalf("open original: %v", err) } - origChunks, err := parseRIFF(origFile) + origChunks, err := riff.Parse(origFile) _ = origFile.Close() if err != nil { @@ -292,7 +375,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) { // Record original chunk data by ID string. origData := map[string][]byte{} for _, c := range origChunks { - origData[string(c.id[:])] = c.data + origData[string(c.ID[:])] = c.Data } // Write a tag to trigger RIFF rewrite. @@ -309,7 +392,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) { t.Fatalf("open after write: %v", err) } - newChunks, err := parseRIFF(newFile) + newChunks, err := riff.Parse(newFile) _ = newFile.Close() if err != nil { @@ -320,7 +403,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) { origNonID3 := 0 for _, c := range origChunks { - if !isID3ChunkID(c.id) { + if !riff.IsID3(c.ID) { origNonID3++ } } @@ -328,7 +411,7 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) { newNonID3 := 0 for _, c := range newChunks { - if !isID3ChunkID(c.id) { + if !riff.IsID3(c.ID) { newNonID3++ } } @@ -359,17 +442,17 @@ func TestWriteWavTags_ChunkPreservation(t *testing.T) { // in chunks and its data matches want byte-for-byte. func checkChunkPreserved( t *testing.T, - chunks []riffChunk, + chunks []riff.Chunk, idStr string, want []byte, ) { t.Helper() for _, c := range chunks { - if string(c.id[:]) == idStr { - if !bytes.Equal(c.data, want) { + if string(c.ID[:]) == idStr { + if !bytes.Equal(c.Data, want) { t.Errorf("chunk %q data changed: got %d bytes, want %d", - idStr, len(c.data), len(want)) + idStr, len(c.Data), len(want)) } return @@ -430,7 +513,7 @@ func TestWriteWavTags_RejectsRF64(t *testing.T) { buf.WriteString("WAVE") // Minimal ds64 chunk (required for RF64 but we just need - // enough bytes for parseRIFF to hit the RF64 rejection). + // enough bytes for riff.Parse to hit the RF64 rejection). buf.WriteString("ds64") _ = binary.Write(&buf, binary.LittleEndian, uint32(28)) //nolint:mnd buf.Write(make([]byte, 28)) //nolint:mnd @@ -454,9 +537,12 @@ func TestWriteWavTags_RejectsRF64(t *testing.T) { // readWavID3Tags extracts ID3v2 metadata from a WAV file by parsing // the RIFF structure and reading the id3 chunk with bogem/id3v2. -// dhowden/tag's ReadFrom does not support WAV files, and its -// ReadID3v2Tags fails on empty tags (after clearing all frames). -// Using bogem/id3v2.ParseReader handles all cases correctly. +// +// metadata.ExtractTags reads a WAV since #104 and is what the round +// trips assert through. This stays for the two cases that are about +// the bytes rather than about the scan: a tag with every frame +// cleared, which no reader reports as anything, and the chunk +// preservation test, which is already parsing the container itself. func readWavID3Tags( t *testing.T, path string, @@ -470,17 +556,17 @@ func readWavID3Tags( defer func() { _ = f.Close() }() - chunks, err := parseRIFF(f) + chunks, err := riff.Parse(f) if err != nil { - t.Fatalf("parseRIFF: %v", err) + t.Fatalf("riff.Parse: %v", err) } // Find the id3 chunk. var id3Data []byte for _, c := range chunks { - if isID3ChunkID(c.id) { - id3Data = c.data + if riff.IsID3(c.ID) { + id3Data = c.Data break } diff --git a/internal/testfixtures/fixtures_test.go b/internal/testfixtures/fixtures_test.go index ba181f8..b440c8c 100644 --- a/internal/testfixtures/fixtures_test.go +++ b/internal/testfixtures/fixtures_test.go @@ -30,12 +30,6 @@ func TestFixturesMatchManifest(t *testing.T) { m := testfixtures.Load(t) for _, want := range m.Tracks { - // WAV tags are write-only today; see - // TestWAVTagsAreNotReadableYet. - if want.Format == "wav" { - continue - } - t.Run(want.Path, func(t *testing.T) { t.Parallel() @@ -161,39 +155,6 @@ func TestDuplicateFixturesAreIndistinguishable(t *testing.T) { } } -// TestWAVTagsAreNotReadableYet pins a known gap rather than hiding it. -// -// backend/tagwriter writes WAV tags into a RIFF "id3 " chunk, but -// backend/metadata reads through dhowden/tag, which recognises MP3, -// FLAC, OGG, MP4 and DSF and has no RIFF parser at all. So every tag -// the app writes to a WAV is invisible to the app that wrote it, and -// WAV tracks always scan in as untitled. -// -// The fixtures are tagged correctly on disk, so when the reader learns -// to unwrap the RIFF chunk this test starts failing — which is the -// point. Delete it then and drop the "wav" skip in -// TestFixturesMatchManifest. -func TestWAVTagsAreNotReadableYet(t *testing.T) { - t.Parallel() - - m := testfixtures.Load(t) - - for _, path := range m.Case(t, testfixtures.CaseWAVTracks) { - got, err := metadata.ExtractTags(path) - if err != nil { - t.Fatalf("extract tags from %s: %v", path, err) - } - - if got.Title != "" { - t.Errorf( - "%s: WAV tags are now readable (%q) — good news; "+ - "see this test's comment for what to update", - filepath.Base(path), got.Title, - ) - } - } -} - func assertTag(t *testing.T, field string, want testfixtures.Track, got string) { t.Helper()