yonlu/yellowjacket
1
1
Fork 0
Code Issues 55 Pull Requests 6 Actions Packages Projects Releases 12 Wiki Activity

WAV tags are written but never read: metadata.ExtractTags cannot see a WAV's id3 chunk #104

New Issue
Closed
opened 2026-08-18 22:18:56 +00:00 by logan · 2 comments
logan commented 2026-08-18 22:18:56 +00:00
Collaborator
Copy Link
Copy Source

Finding

Tripped over while writing the totals round-trip test for #16: the WAV
case of TestWriteTotals_RoundTripsInEveryFormat failed with every
field zero, not just the totals.

tagwriter writes a WAV's tags into a RIFF id3 chunk
(backend/tagwriter/wav.go), correctly — but dhowden/tag, which is
what metadata.ExtractTags is built on, has no RIFF/WAVE reader at
all. There is no wav.go in the module and nothing matches RIFF in
its dispatch. So the scanner cannot see a WAV's tags: not the totals,
not the title, not the artist, not the embedded cover.

Reproduction

path := createTestWAV(t, t.TempDir(), "x.wav", nil)
_ = writeWavTags(testLogger(), path, TagChanges{FieldTitle: "Some Song"})

meta, err := metadata.ExtractTags(path) // err == nil
// meta.Title == ""

Why nothing caught it

backend/tagwriter/wav_test.go verifies its writes by parsing the
id3 chunk itself (readWavID3Tags) rather than by reading the file
back through metadata. That is a reasonable unit test of the writer
and it is why the writer is known-good; it just cannot see that the
other half of the round trip is missing. Every other format's tests go
through metadata.ExtractTags.

Consequence

WAV is in metadata.SupportedFileExtensions, so these files scan and
play. They just import with whatever the filename fallback gives and
stay that way — editing tags on a WAV, autotagging a WAV folder, or
importing a WAV download all appear to succeed and change nothing the
library can see. The file on disk really is tagged; other players will
read it.

Direction

metadata already has a lenient ID3v2 recovery path
(extractID3v2Lenient in tags_lenient.go) that takes a reader and
parses ID3v2 with bogem/id3v2 — the same library the WAV writer
uses. The missing piece is locating the id3 chunk and handing that
reader to it. parseRIFF in backend/tagwriter/wav.go is the parser,
so this is mostly deciding where it should live so both packages can
use it (metadata importing tagwriter is the wrong direction).

Worth measuring first how much WAV is realistically in a library —
this may be Priority/Low on volume alone. Filing it because it is
invisible, not because it is urgent.

**Finding** Tripped over while writing the totals round-trip test for #16: the WAV case of `TestWriteTotals_RoundTripsInEveryFormat` failed with *every* field zero, not just the totals. `tagwriter` writes a WAV's tags into a RIFF `id3 ` chunk (`backend/tagwriter/wav.go`), correctly — but `dhowden/tag`, which is what `metadata.ExtractTags` is built on, **has no RIFF/WAVE reader at all**. There is no `wav.go` in the module and nothing matches `RIFF` in its dispatch. So the scanner cannot see a WAV's tags: not the totals, not the title, not the artist, not the embedded cover. **Reproduction** ```go path := createTestWAV(t, t.TempDir(), "x.wav", nil) _ = writeWavTags(testLogger(), path, TagChanges{FieldTitle: "Some Song"}) meta, err := metadata.ExtractTags(path) // err == nil // meta.Title == "" ``` **Why nothing caught it** `backend/tagwriter/wav_test.go` verifies its writes by parsing the `id3 ` chunk itself (`readWavID3Tags`) rather than by reading the file back through `metadata`. That is a reasonable unit test of the writer and it is why the writer is known-good; it just cannot see that the other half of the round trip is missing. Every other format's tests go through `metadata.ExtractTags`. **Consequence** WAV is in `metadata.SupportedFileExtensions`, so these files scan and play. They just import with whatever the filename fallback gives and stay that way — editing tags on a WAV, autotagging a WAV folder, or importing a WAV download all appear to succeed and change nothing the library can see. The file on disk really is tagged; other players will read it. **Direction** `metadata` already has a lenient ID3v2 recovery path (`extractID3v2Lenient` in `tags_lenient.go`) that takes a reader and parses ID3v2 with `bogem/id3v2` — the same library the WAV writer uses. The missing piece is locating the `id3 ` chunk and handing that reader to it. `parseRIFF` in `backend/tagwriter/wav.go` is the parser, so this is mostly deciding where it should live so both packages can use it (`metadata` importing `tagwriter` is the wrong direction). Worth measuring first how much WAV is realistically in a library — this may be `Priority/Low` on volume alone. Filing it because it is invisible, not because it is urgent.
logan self-assigned this 2026-08-24 09:34:44 +00:00
logan added the
Status
In Progress
 label 2026-08-24 09:34:44 +00:00
logan commented 2026-08-24 09:34:44 +00:00
Author
Collaborator
Copy Link
Copy Source

Picking this up on fix/104-wav-tags-read, from origin/main.

Approach, following the Direction: the RIFF read side moves into a
new backend/riff package, because metadata cannot import
tagwriter in either direction — tagwriter already imports
metadata, so it is an import cycle rather than merely the wrong way
round. backend/tagtotals is the precedent for a tiny package that
exists so two callers need not import each other.

metadata.ExtractTagsFromReader then dispatches on the container: a
RIFF/WAVE file's tags are the id3 chunk, handed to the existing
extractID3v2Lenient, which is already bogem/id3v2 — the same
library the writer uses.

One thing the move is not: parseRIFF reads every chunk's data into
memory, which is what rewriting a file needs and is exactly wrong on
the scan path, where the data chunk is the whole of the audio. So
the read side gets a seek-over-chunks locator beside the existing
parser, sharing one header walker.

totals_test.go's WAV case reads the chunk itself today and its
comment says why; that is the seam the issue's "why nothing caught it"
names, so it goes through metadata.ExtractTags like the other three
formats afterwards.

Not doing the volume measurement the issue suggests: it would decide
the priority, and the priority is already Low.

Picking this up on `fix/104-wav-tags-read`, from `origin/main`. **Approach**, following the Direction: the RIFF read side moves into a new `backend/riff` package, because `metadata` cannot import `tagwriter` in either direction — `tagwriter` already imports `metadata`, so it is an import cycle rather than merely the wrong way round. `backend/tagtotals` is the precedent for a tiny package that exists so two callers need not import each other. `metadata.ExtractTagsFromReader` then dispatches on the container: a RIFF/WAVE file's tags are the `id3 ` chunk, handed to the existing `extractID3v2Lenient`, which is already `bogem/id3v2` — the same library the writer uses. One thing the move is *not*: `parseRIFF` reads every chunk's data into memory, which is what rewriting a file needs and is exactly wrong on the scan path, where the `data` chunk is the whole of the audio. So the read side gets a seek-over-chunks locator beside the existing parser, sharing one header walker. `totals_test.go`'s WAV case reads the chunk itself today and its comment says why; that is the seam the issue's "why nothing caught it" names, so it goes through `metadata.ExtractTags` like the other three formats afterwards. Not doing the volume measurement the issue suggests: it would decide the priority, and the priority is already `Low`.
logan commented 2026-08-24 10:12:16 +00:00
Author
Collaborator
Copy Link
Copy Source

PR #218 — #218 —
CI green (check and e2e, the latter covering chromium and WebKit).

The Direction held, with one correction to its wording: metadata
importing tagwriter is not merely the wrong direction, it is an
import cycle — tagwriter already imports metadata. So the RIFF
read side is a new backend/riff, on backend/tagtotals' precedent.

parseRIFF moved there verbatim as riff.Parse, but the scan path
deliberately does not use it: it holds every chunk's data in
memory, which is what rewriting a file needs, and a WAV's audio is a
chunk — so riff.ID3Chunk seeks over what it is not looking for
instead. Both walk the same headers.

Two decisions the issue did not name. The container is asked before
tag.ReadFrom rather than after it fails, because dhowden's last
resort is an ID3v1 trailer and a WAV carrying both would be read by the
wrong one. And an untagged WAV — no chunk, RF64, a tag with every frame
cleared — reads as empty metadata with no TagReadWarning, since
the filename fallback is the right answer there and a warning would
report a fault on a healthy file.

TestWAVTagsAreNotReadableYet did its job exactly as written: it
failed the moment the reader learned and told me in its own comment
what to update. Deleted, wav skip dropped from
TestFixturesMatchManifest, and totals_test.go's WAV case moved onto
metadata.ExtractTags — which is the seam "why nothing caught it"
names.

Measured on a rebuilt seed: the fixture library's two WAVs now import
as Tone A/Tone B, Field Recordings, Test Tones, Field Recording, 2024, with cover art, where they used to arrive untitled
with no album. make e2e was run for that reason and is green.

Not done, deliberately: the volume measurement the issue suggests — it
would decide the priority and the priority is already Low. Two
findings filed instead of folded in: #216 (riff.Parse allocates
whatever size a chunk header claims — writer path only) and #217
(an e2e comment that names these two WAVs as the library's albumless
tracks, which they no longer are).

**PR #218** — https://git.ljones.me/yonlu/yellowjacket/pulls/218 — CI green (`check` and `e2e`, the latter covering chromium and WebKit). The Direction held, with one correction to its wording: `metadata` importing `tagwriter` is not merely the wrong direction, it is an **import cycle** — `tagwriter` already imports `metadata`. So the RIFF read side is a new `backend/riff`, on `backend/tagtotals`' precedent. `parseRIFF` moved there verbatim as `riff.Parse`, but the scan path deliberately does **not** use it: it holds every chunk's data in memory, which is what rewriting a file needs, and a WAV's audio *is* a chunk — so `riff.ID3Chunk` seeks over what it is not looking for instead. Both walk the same headers. Two decisions the issue did not name. The container is asked **before** `tag.ReadFrom` rather than after it fails, because dhowden's last resort is an ID3v1 trailer and a WAV carrying both would be read by the wrong one. And an untagged WAV — no chunk, RF64, a tag with every frame cleared — reads as empty metadata with **no** `TagReadWarning`, since the filename fallback is the right answer there and a warning would report a fault on a healthy file. `TestWAVTagsAreNotReadableYet` did its job exactly as written: it failed the moment the reader learned and told me in its own comment what to update. Deleted, `wav` skip dropped from `TestFixturesMatchManifest`, and `totals_test.go`'s WAV case moved onto `metadata.ExtractTags` — which is the seam "why nothing caught it" names. Measured on a rebuilt seed: the fixture library's two WAVs now import as `Tone A`/`Tone B`, `Field Recordings`, `Test Tones`, `Field Recording`, 2024, with cover art, where they used to arrive untitled with no album. `make e2e` was run for that reason and is green. Not done, deliberately: the volume measurement the issue suggests — it would decide the priority and the priority is already `Low`. Two findings filed instead of folded in: **#216** (`riff.Parse` allocates whatever size a chunk header claims — writer path only) and **#217** (an e2e comment that names these two WAVs as the library's albumless tracks, which they no longer are).
logan closed this issue 2026-08-25 16:37:56 +00:00
gitea-actions bot removed the
Status
In Progress
 label 2026-08-25 16:38:17 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
Area/Design
Area/Downloads
Area/Explore
Area/Library-UI
Area/Metadata
Area/Packaging
Area/Player
Area/Queue
Area/Settings
Area/Shell-Nav
Compat/Breaking
Breaking change that won't be backward compatible
 Kind/Bug
Something is not working
 Kind/Documentation
Documentation changes
 Kind/Enhancement
Improve existing functionality
 Kind/Feature
New functionality
 Kind/Security
This is security issue
 Kind/Testing
Issue or pull request related to testing
 Platform/Android
Platform/Desktop
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
1
Issue has been confirmed
Reviewed
Duplicate
2
This issue or pull request already exists
Reviewed
Invalid
3
Invalid issue
Reviewed
Won't Fix
3
This issue won't be fixed
Status
Blocked
1
Something is blocking this issue or pull request
Status
Need More Info
2
Feedback is required to reproduce issue or to continue work
Status
Abandoned
3
Somebody has started to work on this but abandoned work
Status
In Progress
Somebody is actively working on this right now
No labels Area/Metadata Kind/Bug Platform/Desktop
Priority
Low
4
Reviewed
Confirmed
1
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logan yonlu
No Assignees logan
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: yonlu/yellowjacket#104