docs: complete project research for v1.2.1 Format Parity
This commit is contained in:
+279
-221
@@ -1,382 +1,440 @@
|
||||
# Domain Pitfalls: Tag Editing
|
||||
# Domain Pitfalls: OGG Vorbis + WAV Tag Writing
|
||||
|
||||
**Domain:** Adding tag editing to an existing music player with normalized DB
|
||||
**Researched:** 2026-03-16
|
||||
**Confidence:** HIGH (based on codebase analysis + format specifications + SQLite FTS5 docs)
|
||||
**Domain:** Adding OGG Vorbis and WAV tag writing to an existing tag writing pipeline (MP3 + FLAC already working)
|
||||
**Researched:** 2026-03-18
|
||||
**Confidence:** HIGH (OGG spec + RFC 3533 + codebase analysis + Xiph Vorbis comment spec + RIFF spec)
|
||||
|
||||
---
|
||||
|
||||
## Critical Pitfalls
|
||||
|
||||
Mistakes that cause data loss, file corruption, or require architectural rework.
|
||||
Mistakes that cause file corruption, unplayable audio, or require significant rework.
|
||||
|
||||
### P1: FLAC Tag Writes Require Full File Rewrite
|
||||
### P1: OGG CRC32 Recalculation After Page Modification
|
||||
|
||||
**What goes wrong:** FLAC stores Vorbis Comments in a METADATA_BLOCK after the STREAMINFO block. Unlike MP3 (which has padding in ID3v2 headers), FLAC metadata blocks are tightly packed with no padding by default. Changing a tag that increases the metadata size requires rewriting the entire file — moving every audio frame forward. A crash or power loss during this rewrite corrupts the file irrecoverably.
|
||||
**Severity:** BLOCKS SHIP — corrupted files won't play
|
||||
**What goes wrong:** Every OGG page contains a CRC32 checksum (bytes 22–25 of the page header) computed over the entire page (header with CRC field zeroed + segment table + page data). The polynomial is `0x04c11db7` but uses a **non-standard bit ordering** — it's a direct algorithm (MSB-first), NOT the common CRC32 used in zlib/Ethernet (which is reflected/LSB-first). If you use Go's standard `hash/crc32` package with `crc32.MakeTable(0x04c11db7)`, you get the **wrong CRC** because `hash/crc32` uses reflected bit ordering.
|
||||
|
||||
**Why it happens:** FLAC spec doesn't mandate padding blocks. Most FLAC files in the wild have zero padding. Even if padding exists, adding cover art (which can be 100KB+) almost always exceeds it.
|
||||
**Why it happens:** Developers see "CRC32 with polynomial 0x04c11db7" and reach for the standard library. OGG uses a non-reflected (direct) CRC32 which is different from IEEE CRC32 despite sharing the same polynomial constant. The dhowden/tag source code (ogg.go) and jfreymuth/oggvorbis (crc.go) both implement custom CRC32 lookup tables for this reason.
|
||||
|
||||
**Consequences:** Corrupted FLAC files that won't play. Audio data intact on disk but offset table is wrong, so decoders can't find frames.
|
||||
**Consequences:** Every OGG decoder will reject pages with incorrect CRC. The file appears corrupted. Players show "codec error" or silence. Some players may try to resync and play garbled audio.
|
||||
|
||||
**Prevention:**
|
||||
1. **Write-to-temp-then-rename (mandatory for all formats).** Write modified file to a temp file in the same directory (same filesystem), then `os.Rename()` atomically. This is already listed in PROJECT.md as a target feature.
|
||||
2. For FLAC specifically: read entire file → write new metadata blocks → copy audio frames → rename. There is no in-place shortcut.
|
||||
3. Verify the written file can be opened and has correct duration before replacing the original.
|
||||
4. Consider adding a PADDING metadata block after writing (e.g. 8KB) so small future edits can be done in-place. This is what tools like `metaflac` do.
|
||||
1. **Copy the CRC implementation from jfreymuth/oggvorbis/crc.go** — it's already an indirect dependency and has the correct OGG CRC32 table (MSB-first, polynomial 0x04c11db7).
|
||||
2. **Never use `hash/crc32` from the standard library** for OGG pages.
|
||||
3. Compute CRC with the checksum field set to zero in the header bytes, then write the computed CRC into bytes 22–25.
|
||||
4. **Round-trip test:** write file → verify every page's CRC matches by re-reading with a CRC-checking reader.
|
||||
|
||||
**Detection:** File size changes unexpectedly; beep decoder fails to open the file after write; duration changes after write (offset corruption).
|
||||
**Detection:** After writing, re-read every page and verify CRC matches. This should be a unit test, not just manual verification.
|
||||
|
||||
**Phase:** File write layer (earliest phase)
|
||||
**Phase:** OGG writer core (first thing to get right)
|
||||
|
||||
---
|
||||
|
||||
### P2: Currently-Playing File Cannot Be Written On Windows (And Shouldn't On Any Platform)
|
||||
### P2: OGG Page Sequence Number Continuity
|
||||
|
||||
**What goes wrong:** The player holds an `os.File` handle on the currently playing track (`p.currentFile` in `player.go:461`). On Windows, the OS enforces mandatory file locking — `os.Rename()` will fail with "The process cannot access the file because it is being used by another process." On Linux, the rename succeeds but the player continues reading the old inode (now unlinked), which works until something closes and reopens the path.
|
||||
**Severity:** BLOCKS SHIP — seeking and error recovery break
|
||||
**What goes wrong:** When rewriting OGG pages (because the comment header packet changed size), all subsequent page sequence numbers must be recalculated to remain strictly sequential (0, 1, 2, 3, ...). If you modify the comment header and it now spans a different number of pages, every page after it in the stream has a wrong sequence number. OGG decoders use sequence numbers for page loss detection and seeking.
|
||||
|
||||
**Why it happens:** The player opens files with `os.Open()` and holds them open for the duration of playback (streaming audio data). The beep library reads from this file handle continuously.
|
||||
**Why it happens:** The comment header is typically in page 1 (page 0 = BOS with codec identification). If the comment data grows large enough to span multiple pages, or shrinks from spanning multiple to fitting in one, the total page count changes, and all subsequent audio pages need renumbering.
|
||||
|
||||
**Consequences:** On Windows: tag write fails silently or with confusing error. On Linux: tag write succeeds but the player sees stale data, and if the user seeks, the streamer may read garbage from the new file at old offsets.
|
||||
**Consequences:** Players report "pages missing" or "page sequence gap." Seeking to specific positions fails. Some decoders abort playback entirely. Others may play but report errors in the log.
|
||||
|
||||
**Prevention:**
|
||||
1. **Check if the target file is currently playing before writing.** Compare `player.currentTrackPath` against the edit target.
|
||||
2. If the file IS playing: stop playback, close the file handle, perform the write, then reload and seek to the previous position. This creates a brief audio glitch but is the only safe approach.
|
||||
3. For batch edits that include the current track: edit all other files first, handle the playing file last with the stop-write-reload dance.
|
||||
4. Alternative (simpler): refuse to edit the currently playing file and show a user-facing message. Less ideal UX but avoids complexity.
|
||||
1. **Full-stream rewrite approach:** Read all pages → replace comment packet → regenerate all pages with correct sequence numbers → write as new file. This is the safest approach and matches how FLAC rewrite already works.
|
||||
2. After rewriting, verify: page 0 has sequence 0, page 1 has sequence 1, etc.
|
||||
3. The approach of "only rewrite the comment pages and shift the rest" is theoretically possible but fragile — the full rewrite is correct-by-construction.
|
||||
|
||||
**Detection:** `os.Rename()` returns error on Windows. On Linux, no error but playback becomes corrupted after seek.
|
||||
**Detection:** After writing, scan through all pages and verify monotonically increasing sequence numbers starting from 0.
|
||||
|
||||
**Phase:** File write layer + player integration
|
||||
**Phase:** OGG writer core
|
||||
|
||||
---
|
||||
|
||||
### P3: FTS5 Contentless Table Cannot UPDATE or DELETE Individual Rows
|
||||
### P3: OGG Granule Position Corruption
|
||||
|
||||
**What goes wrong:** The current `search_index` is a contentless FTS5 table (`content=''`). The existing `DeleteSearchIndex()` method is literally a no-op (see `search.go:120-127`). After editing a track's title from "Love Song" to "Heart Song", searching for "Love Song" still returns the track because the old FTS5 entry cannot be removed. The stale entry points to a valid rowid, and the JOIN against `track_metadata` will return the row (now with different data), so the user sees a search result that doesn't match their query.
|
||||
**Severity:** BLOCKS SHIP — playback duration and seeking wrong
|
||||
**What goes wrong:** Each OGG page has a 64-bit absolute granule position field (bytes 6–13) that encodes the total decoded samples up to and including the last completed packet on that page. If you rewrite pages and accidentally modify granule positions, seeking breaks and the reported duration is wrong. A special value of -1 (0xFFFFFFFFFFFFFFFF) means "no packets complete on this page."
|
||||
|
||||
**Why it happens:** Contentless FTS5 (`content=''`) stores only the index, not the original text. Without the original text, FTS5 can't compute what tokens to remove from the index. The current design relies on full rebuilds during rescan, which is fine for the read-only case but breaks for incremental edits.
|
||||
**Why it happens:** When splitting or merging pages during rewrite, it's easy to accidentally assign granule positions to pages that contain the comment header (which should have granule position 0 for the first two pages per the Vorbis spec) or to shift granule positions of audio pages.
|
||||
|
||||
**Consequences:** Search returns false positives after tag edits. The more edits the user makes, the worse search quality gets — until the next full rescan rebuilds the index.
|
||||
**Consequences:** Player reports wrong track duration. Seeking jumps to wrong positions. Progress bar is inaccurate.
|
||||
|
||||
**Prevention — Two Options:**
|
||||
**Prevention:**
|
||||
1. **Header pages (BOS page + comment page(s)) MUST have granule position 0.** This is mandated by the Vorbis I spec.
|
||||
2. **Audio page granule positions must be preserved exactly as-is** from the original file. Since we're only modifying the comment header, audio data doesn't change.
|
||||
3. The full-rewrite approach: copy BOS page (granule=0), write new comment pages (granule=0), then copy all remaining audio pages verbatim (preserving their original granule positions but updating sequence numbers and CRCs).
|
||||
|
||||
**Option A: Migrate to `contentless_delete=1` (Recommended)**
|
||||
SQLite 3.43.0+ supports `contentless_delete=1` which enables DELETE and INSERT OR REPLACE. This requires a schema migration (drop + recreate the FTS5 table). The `modernc.org/sqlite` driver bundles SQLite 3.45+, so this is available. After migration, tag edit can do: DELETE the old row, INSERT the new row. This is the `DELETE + INSERT` pattern already noted in the milestone context.
|
||||
**Detection:** Compare granule positions of audio pages before and after writing.
|
||||
|
||||
**Option B: Rebuild the entire index after each edit session**
|
||||
Call `RebuildSearchIndex()` after completing all tag writes. This is expensive (reads all tracks) but correct. Could be batched — rebuild once after a batch edit, not per-track.
|
||||
|
||||
**Recommendation:** Option A. The migration is straightforward and makes individual updates O(1) instead of O(n). The existing `RebuildSearchIndex()` becomes the migration step.
|
||||
|
||||
**Detection:** Search for old tag values — if they return results with the new values, the index is stale.
|
||||
|
||||
**Phase:** Schema migration (do first, before any tag write code)
|
||||
**Phase:** OGG writer core
|
||||
|
||||
---
|
||||
|
||||
### P4: Shared Entity Fan-Out — Editing Artist on One Track Affects Zero or Fifty Others
|
||||
### P4: OGG Vorbis Packet Structure — Three-Header Requirement
|
||||
|
||||
**What goes wrong:** The normalized schema shares entities across tracks. An `artist_credit` row with text "The Beatles" may be referenced by 200 recordings via `recordings.artist_credit_id`. If the user edits the artist field on one track from "The Beatles" to "Beatles, The", the system must decide: (a) update the shared `artist_credit` row (changing all 200 tracks), (b) create a new `artist_credit` and repoint only this track's recording, or (c) something else.
|
||||
**Severity:** BLOCKS SHIP — file unplayable if headers are wrong
|
||||
**What goes wrong:** A Vorbis stream in OGG has exactly three header packets in order: (1) identification header (starts with `\x01vorbis`), (2) comment header (starts with `\x03vorbis`), (3) setup header (starts with `\x05vorbis`). The identification header MUST be alone on the first page (BOS page). The comment and setup headers MUST appear before any audio data and MUST begin on the second page. If the rewriter corrupts the packet boundary between the comment and setup headers, the decoder can't initialize.
|
||||
|
||||
**Why it happens:** The MusicBrainz-inspired schema (`artists` → `artist_credit` → `recordings`) is designed for read-heavy workloads where entities are shared. Tag editing breaks this assumption by making per-track changes that may or may not be intended as global changes.
|
||||
**Why it happens:** The comment header can span multiple pages (especially with large cover art). The setup header immediately follows in the same stream of pages. If you modify the comment header's size, the boundary between comment and setup packets shifts. If you re-segment into pages incorrectly, the setup header may be split wrong.
|
||||
|
||||
**Consequences:**
|
||||
- If you update the shared row: user edits one track, 199 other tracks silently change. Terrifying.
|
||||
- If you create new rows: orphaned entities accumulate (old `artist_credit` row with only 199 refs, then 198, etc.). The artist browse view shows "The Beatles" AND "Beatles, The" as separate entries.
|
||||
- If you try to be smart about it: complex merge/split logic that's hard to get right.
|
||||
**Consequences:** Decoder fails to initialize. File appears to be an invalid Vorbis stream.
|
||||
|
||||
**Prevention:**
|
||||
1. **Tag editing always creates new entity rows for the edited track.** Create a new `recording`, new `artist_credit` (if changed), new `release_group_recordings` link, new `genre_recordings` links. Point the `audio_file.recording_id` at the new recording. This is the safest approach and matches what the scan pipeline already does (it always creates new recordings).
|
||||
2. **Orphan cleanup after edit.** After repointing the audio_file, check if the old recording is still referenced by any audio_file. If not, delete it (and cascade to its genre links, release_group links). Same for artist_credit, artists, genres, release_groups.
|
||||
3. **Never mutate shared entities in-place** during single-track or batch-within-same-album editing. The only exception is intentional "rename this artist across all tracks" which should be a separate, explicit feature (not part of v1.2).
|
||||
1. Parse the original stream to identify exactly where each header packet starts and ends.
|
||||
2. Replace only the comment packet data. Preserve identification and setup packets byte-for-byte.
|
||||
3. When re-assembling into pages: page 0 = BOS with identification header only. Page 1+ = comment header + setup header (they can share pages, but identification must be alone on page 0).
|
||||
4. After writing, verify all three header packets are parseable.
|
||||
|
||||
**Detection:** After editing one track's artist, check if other tracks in the same album now show the wrong artist.
|
||||
|
||||
**Phase:** Database update layer (core architecture decision — must be settled before writing any DB update code)
|
||||
**Phase:** OGG writer core
|
||||
|
||||
---
|
||||
|
||||
### P5: Race Condition — Scan Runs While Tags Are Being Written
|
||||
### P5: WAV ID3v2 vs RIFF INFO — Choosing Wrong Metadata Format
|
||||
|
||||
**What goes wrong:** User starts editing tags. While the edit is in progress (writing files, updating DB), a library scan starts (either from the scan queue, soft scan on launch, or user-initiated). The scan reads the file's tags (which may be half-written or already-written-but-DB-not-yet-updated), creates new entity rows, and overwrites the DB state that the tag editor just carefully set up.
|
||||
**Severity:** BLOCKS SHIP — metadata not readable by players or our own reader
|
||||
**What goes wrong:** WAV files can contain metadata in multiple formats: RIFF INFO chunks (LIST/INFO), ID3v2 tags (`id3 ` or `ID3 ` chunk), or both. dhowden/tag reads **ID3v2 tags from WAV files** (it looks for the `ID3` marker inside RIFF chunks). If we write RIFF INFO tags but dhowden/tag reads ID3v2, our written tags won't round-trip through our own reader. If we write ID3v2 but the file already has RIFF INFO, players that prefer RIFF INFO will show old data.
|
||||
|
||||
**Why it happens:** The scan pipeline (`scanInternal`) and tag editing are independent operations. The scan loads existing files from DB, walks the filesystem, extracts metadata, and writes to DB. If a file's on-disk tags differ from the DB (because the edit just wrote new tags), the scan treats it as needing an update and overwrites the recording.
|
||||
**Why it happens:** There's no single WAV metadata standard. The music production world uses RIFF INFO (INAM, IART, etc.), while the consumer audio world often uses ID3v2 embedded in WAV. Different tools write different formats. dhowden/tag's WAV support reads ID3v2 tags.
|
||||
|
||||
**Consequences:** Tag edits silently reverted. Or worse: the scan creates duplicate recordings (one from the edit, one from the scan) because the scan's entity cache doesn't know about the edit's newly-created entities.
|
||||
**Consequences:** Tags appear written but don't show up when re-reading the file. Or worse, conflicting metadata between ID3v2 and RIFF INFO confuses players.
|
||||
|
||||
**Prevention:**
|
||||
1. **Mutual exclusion between tag editing and scanning.** While tag writes are in progress, block scan start (or vice versa). The existing `l.mu` mutex protects scan state; extend it to cover "edit in progress" state.
|
||||
2. **Simpler: Use the existing scan queue coordinator.** Tag edits happen on the main goroutine (via Wails binding). Scans run in background goroutines. Since SQLite has `SetMaxOpenConns(1)`, DB writes are already serialized. The risk is the scan re-reading the file AFTER the tag write but BEFORE the DB update. Solution: perform the file write and DB update atomically (in the same critical section), and have the scan skip files that were recently edited (timestamp check or "edited" flag).
|
||||
3. **Best approach: Pause/cancel active scan during tag edit, resume after.** The existing `PauseScan()`/`ResumeScan()` mechanism can be leveraged. Pause the scan, do the edit (file write + DB update), resume the scan.
|
||||
1. **Write ID3v2 tags in WAV files** because that's what dhowden/tag reads back. Use the same `bogem/id3v2/v2` library already used for MP3 tag writing.
|
||||
2. WAV ID3v2 approach: the `id3 ` chunk in RIFF contains a complete ID3v2 tag. Write the ID3v2 tag into this chunk.
|
||||
3. If the file has existing RIFF INFO tags, **leave them alone** — don't delete them, don't try to sync them. Only modify the ID3v2 chunk.
|
||||
4. Round-trip test: write via our writer → read via `metadata.ExtractTags` → verify all fields match.
|
||||
|
||||
**Detection:** Edit a tag, immediately trigger a scan, check if the edit survives.
|
||||
**Detection:** Write a tag, then immediately read it back with dhowden/tag. If any field doesn't round-trip, the format choice is wrong.
|
||||
|
||||
**Phase:** Tag write integration with scan pipeline
|
||||
**Phase:** WAV writer core
|
||||
|
||||
---
|
||||
|
||||
### P6: Temp File Rename Fails Across Filesystem Boundaries
|
||||
### P6: WAV RIFF Chunk Size Updates
|
||||
|
||||
**What goes wrong:** `os.Rename()` is atomic only when source and dest are on the same filesystem. If the temp file is created in `/tmp` (default `os.CreateTemp` behavior) but the music file is on `/mnt/music`, the rename becomes a copy+delete — no longer atomic, and if interrupted, you lose the file.
|
||||
**Severity:** BLOCKS SHIP — file unplayable if sizes are wrong
|
||||
**What goes wrong:** WAV is a RIFF container where every chunk has a 4-byte ID + 4-byte little-endian size. The outermost RIFF chunk's size field must equal the total file size minus 8 bytes. If you add or resize the `id3 ` chunk but don't update the outer RIFF size, the file appears truncated to parsers. Some players ignore the RIFF size and read to EOF, but others (including some decoders) stop at the declared size.
|
||||
|
||||
**Why it happens:** Many developers use `os.CreateTemp("", ...)` which defaults to the system temp directory, which is often a different filesystem/partition from where music files live.
|
||||
**Why it happens:** When adding an ID3v2 chunk to a WAV that didn't have one, or resizing an existing chunk, you need to update not just the chunk's own size but also the parent RIFF size and possibly the `data` chunk boundaries.
|
||||
|
||||
**Consequences:** Non-atomic write. Power loss during copy = corrupted or missing file.
|
||||
**Consequences:** File appears truncated. Some players play it fine (they read to EOF), others refuse to open it or play only partial audio.
|
||||
|
||||
**Prevention:**
|
||||
1. **Create the temp file in the same directory as the target file.** Use `os.CreateTemp(filepath.Dir(targetPath), ".yj-edit-*")` to ensure same-filesystem rename.
|
||||
2. Clean up temp files on startup (find files matching `.yj-edit-*` pattern in library directories — these are orphaned from crashed edits).
|
||||
3. Use the temp file pattern: `<dir>/.yj-edit-<random>` → write → `os.Rename()` → done. If rename fails, the temp file is deleted. The original is untouched.
|
||||
1. **Full-file rewrite approach (like FLAC):** Read original → write RIFF header → write `fmt ` chunk → write `data` chunk → write other chunks → write `id3 ` chunk → fix RIFF size. AtomicWrite handles crash safety.
|
||||
2. Calculate final RIFF size as sum of all chunk sizes + their 8-byte headers + 4 bytes for "WAVE" form type.
|
||||
3. Place the `id3 ` chunk **after** the `data` chunk, not before it. Putting metadata before audio data means players must seek past it, and some naive parsers may not handle it.
|
||||
4. Test with both small WAV files (~1KB) and medium WAV files (~50MB) to catch size calculation bugs.
|
||||
|
||||
**Detection:** Check if `os.Rename()` returns `EXDEV` (cross-device link) error.
|
||||
**Detection:** After writing, verify: `file_size == RIFF_size + 8`. Open with beep's wav.Decode to verify playback still works.
|
||||
|
||||
**Phase:** File write layer (earliest phase)
|
||||
**Phase:** WAV writer core
|
||||
|
||||
---
|
||||
|
||||
## Moderate Pitfalls
|
||||
|
||||
Mistakes that cause bugs, degraded UX, or significant rework.
|
||||
Mistakes that cause incorrect behavior, data loss in edge cases, or significant debugging time.
|
||||
|
||||
### P7: ID3v2 Encoding Mismatch — UTF-8 Written Where Latin-1 Expected
|
||||
### P7: OGG Vorbis Comment Framing Bit
|
||||
|
||||
**What goes wrong:** ID3v2.3 (the most common version) defaults to ISO-8859-1 (Latin-1) encoding for text frames. If the tag writing library writes UTF-8 text into a Latin-1 frame without setting the encoding byte to UTF-8/UTF-16, players that strictly follow the spec will display garbled text (mojibake). Conversely, some players write Latin-1 tags that `dhowden/tag` reads as UTF-8, causing garbled reads.
|
||||
**Severity:** HIGH — file won't read if framing bit is wrong
|
||||
**What goes wrong:** The Vorbis comment header (inside OGG Vorbis streams) ends with a mandatory **framing bit**. This is a single `1` bit at the end of the comment data, byte-aligned. If you construct the comment header packet without this framing bit, Vorbis decoders will reject the header. The Vorbis I spec says: "if framing_bit unset or end of packet then ERROR."
|
||||
|
||||
**Why it happens:** ID3v2.3 only officially supports ISO-8859-1 and UTF-16. UTF-8 support was added in ID3v2.4. Many real-world files are ID3v2.3 with UTF-8 text (spec violation that most players tolerate). When writing tags, the library must match the encoding scheme to the ID3v2 version.
|
||||
|
||||
**Consequences:** Non-ASCII characters (accents, CJK, Cyrillic) display as garbage in other players after editing with YellowJacket.
|
||||
**Why it happens:** The OGG Opus format does NOT have this framing bit (OpusTags format omits it). If you're looking at Opus examples or generic Vorbis comment code, you might omit it. The framing bit is Vorbis-specific, not a general Vorbis Comment feature.
|
||||
|
||||
**Prevention:**
|
||||
1. **Use `bogem/id3v2` (aka `n10v/id3v2`) for MP3 tag writing.** This library handles encoding correctly — it auto-selects UTF-8 for v2.4 and UTF-16 for v2.3, or allows explicit control.
|
||||
2. When writing ID3v2.3 tags with non-ASCII content, use UTF-16 encoding (the only Unicode encoding ID3v2.3 supports).
|
||||
3. Consider upgrading all written tags to ID3v2.4 (which supports UTF-8 natively). This is what most modern taggers do.
|
||||
4. **Read the existing tag version and preserve it** unless the user explicitly requests an upgrade.
|
||||
1. After writing vendor string + comment count + all comments, write a byte `0x01` (framing bit set in LSB position).
|
||||
2. If reusing flacvorbis library to build the comment data: **FLAC Vorbis Comments do NOT have a framing bit** (FLAC metadata blocks have their own length framing). So you can't just take the FLAC Vorbis comment bytes and paste them into an OGG packet — you need to add the framing bit.
|
||||
3. Test: write a file, then verify `oggvorbis.GetCommentHeader()` successfully parses it.
|
||||
|
||||
**Detection:** Edit a track with non-ASCII characters, open in another player (VLC, foobar2000), check for garbled text.
|
||||
|
||||
**Phase:** Tag writing layer
|
||||
**Phase:** OGG writer
|
||||
|
||||
---
|
||||
|
||||
### P8: Cover Art Embedding Size and Format Incompatibilities
|
||||
### P8: OGG Vorbis Comment Packet Prefix
|
||||
|
||||
**What goes wrong:**
|
||||
- **JPEG vs PNG:** Both ID3v2 and FLAC Vorbis Comments support JPEG and PNG cover art. However, some older players only handle JPEG. If the user selects a PNG, it should work but may not display in all contexts.
|
||||
- **Image size:** Users may select a 10MB PNG file as cover art. Embedding this in every track of a 50-track album creates 500MB of overhead. The file write becomes extremely slow, and the FLAC rewrite (P1) is even worse because the entire file must be rewritten.
|
||||
- **FLAC cover art is stored as a PICTURE metadata block** with specific structure (picture type, MIME type, description, width, height, color depth, data). Getting any of these fields wrong causes players to not display the art.
|
||||
- **Vorbis Comments in OGG:** Cover art in OGG Vorbis files is stored as a base64-encoded METADATA_BLOCK_PICTURE in a Vorbis Comment field. This is a different mechanism than FLAC's native PICTURE block, despite both using "Vorbis Comments."
|
||||
**Severity:** HIGH — comment header not recognized
|
||||
**What goes wrong:** In OGG Vorbis, the comment header packet must start with the 7-byte prefix `\x03vorbis`. In OGG Opus, it starts with `OpusTags` (8 bytes). If you omit this prefix or use the wrong one, the reader won't find the comment header. dhowden/tag checks for both prefixes (`\x03vorbis` and `OpusTags`) to dispatch to Vorbis comment parsing.
|
||||
|
||||
**Consequences:** Cover art doesn't display in other players. Enormous file size increase. Slow writes.
|
||||
**Why it happens:** Confusing OGG Vorbis with OGG Opus. Or confusing FLAC Vorbis Comments (which have no packet prefix, they're identified by metadata block type) with OGG Vorbis Comments.
|
||||
|
||||
**Prevention:**
|
||||
1. **Resize cover art before embedding.** Cap at 800x800 or 1000x1000 pixels. Convert to JPEG (quality 90) for embedding — better compression than PNG for photos.
|
||||
2. **Validate image before embedding.** Decode it, check dimensions, re-encode if needed. Use `image/jpeg` and `image/png` standard library packages (already in use for thumbnail generation via `golang.org/x/image`).
|
||||
3. **For FLAC:** Populate ALL required PICTURE block fields (picture type=3 "front cover", MIME type, width, height, bit depth, data).
|
||||
4. **For OGG:** Base64-encode the FLAC PICTURE block structure into a `METADATA_BLOCK_PICTURE` Vorbis Comment field.
|
||||
5. **Show file size impact preview** in the UI before confirming cover art change on batch operations.
|
||||
1. When building the comment packet for OGG Vorbis: prepend `\x03vorbis` (7 bytes) before the Vorbis Comment data.
|
||||
2. The comment data itself (vendor string, comment list) is identical in format to what FLAC uses.
|
||||
3. **Scope decision:** For v1.2.1, only support `.ogg` files containing Vorbis (prefix `\x01vorbis` on identification header). Do NOT attempt to handle Opus files (`.opus`) — that's a different format with different header structure.
|
||||
|
||||
**Detection:** Embed cover art, open in another player, check if art displays. Check file size increase.
|
||||
|
||||
**Phase:** Cover art write layer
|
||||
**Phase:** OGG writer
|
||||
|
||||
---
|
||||
|
||||
### P9: Batch Edit Creates Hundreds of Orphaned Entity Rows
|
||||
### P9: Cover Art in OGG Vorbis — METADATA_BLOCK_PICTURE
|
||||
|
||||
**What goes wrong:** User selects 50 tracks from an album and changes the artist name. Following P4's approach (create new entities, repoint audio_file), this creates 50 new recordings, 1 new artist_credit, and 50 new release_group_recordings links. The old recording rows (and their genre links) are now orphaned — nothing references them. Without cleanup, the artists/albums/genres views show ghost entries.
|
||||
**Severity:** MEDIUM-HIGH — cover art either too large or not recognized
|
||||
**What goes wrong:** Vorbis Comments don't have a dedicated picture block like FLAC. Instead, cover art is stored as a `METADATA_BLOCK_PICTURE` comment field whose value is a **base64-encoded** FLAC PICTURE block. A typical 500KB JPEG becomes ~680KB of base64 text in a single Vorbis Comment entry. This dramatically increases the comment header size, which means the comment header may span many OGG pages (each page is max ~64KB). Additionally, some players only support this format while others look for a `COVERART` field (deprecated).
|
||||
|
||||
**Why it happens:** The "always create new" approach from P4 is correct for safety but generates garbage. The existing scan pipeline never updates entities — it only creates them. There's no existing orphan cleanup for recordings/artists/genres (only for audio_files during scan).
|
||||
**Why it happens:** The Vorbis Comment spec has no native image support. The community adopted the FLAC PICTURE structure encoded as base64 in a comment field, but it's cumbersome and space-inefficient.
|
||||
|
||||
**Consequences:** Ghost artists, albums, and genres appear in browse views. Database grows over time. Genre list fills with duplicates if genre spelling varies slightly across edits.
|
||||
**Consequences:**
|
||||
- A 1MB cover image becomes ~1.36MB of base64, which needs ~21 OGG pages just for the comment header.
|
||||
- Large comment headers stress the page segmentation code — more pages means more CRC calculations, more sequence number management.
|
||||
- If you use the deprecated `COVERART` field instead, dhowden/tag won't read it.
|
||||
|
||||
**Prevention:**
|
||||
1. **Run entity orphan cleanup after every edit (or batch edit).** In a single transaction:
|
||||
- Delete recordings not referenced by any audio_file
|
||||
- Delete release_group_recordings referencing deleted recordings
|
||||
- Delete recording_genres referencing deleted recordings
|
||||
- Delete artist_credits not referenced by any recording or release_group
|
||||
- Delete artists not referenced by any artist_credit_artist
|
||||
- Delete genres not referenced by any recording_genres
|
||||
- Delete release_groups not referenced by any release_group_recordings
|
||||
- Delete cover_art not referenced by any release_groups
|
||||
2. **Use `LEFT JOIN ... WHERE ... IS NULL` pattern** (same approach documented in P4 of the multi-library PITFALLS).
|
||||
3. **Batch the cleanup** — run once per edit session, not per-track.
|
||||
1. Use the `METADATA_BLOCK_PICTURE` field name.
|
||||
2. Value format: base64-encode a FLAC PICTURE structure (picture type + MIME type + description + width/height/depth/colors + image data). The `go-flac/flacpicture` library already knows this format — marshal a `flacpicture.MetadataBlockPicture`, then base64-encode the result.
|
||||
3. **Consider a max cover art size limit** (e.g., 2MB) to avoid pathological page counts. Log a warning for large images.
|
||||
4. Test round-trip: write cover art → read back with dhowden/tag → verify bytes match.
|
||||
|
||||
**Detection:** After batch edit, check that the old artist/album/genre no longer appears in browse views (unless other tracks still reference them).
|
||||
|
||||
**Phase:** Database update layer (immediately after P4's approach is implemented)
|
||||
**Phase:** OGG writer (possibly deferred from initial implementation if too complex)
|
||||
|
||||
---
|
||||
|
||||
### P10: Genre Storage Mismatch — Comma-Separated String vs Multi-Value
|
||||
### P10: WAV Chunk Word Alignment
|
||||
|
||||
**What goes wrong:** The `recordings.genre` column stores genre as a free-text string. The existing `metadata.ParseGenres()` splits on `,` and `;` and normalizes to title case. But the `recording_genres` M:N junction table stores individual genre links. These two representations can diverge: the string says "Rock, Pop" but the junction table has links to "Rock" and "Pop" as separate genre entities. After a tag edit, if only the string is updated (or only the junction table), they fall out of sync.
|
||||
**Severity:** MEDIUM — some parsers fail on unaligned chunks
|
||||
**What goes wrong:** The RIFF specification requires chunks to start at even byte offsets (word-aligned). If a chunk has an odd-length data section, a padding byte (`0x00`) must be added after the data before the next chunk begins. This padding byte is NOT included in the chunk's size field. If you write an `id3 ` chunk with odd-length data and don't add the pad byte, the next chunk's header will be misaligned and parsers will fail to find it.
|
||||
|
||||
**Why it happens:** Dual representation — the raw string in `recordings.genre` and the normalized M:N links in `recording_genres`. The scan pipeline populates both, but an edit might only update one.
|
||||
**Why it happens:** Easy to forget the padding byte, especially since the size field doesn't include it. Many existing WAV files have this bug (written by buggy software), so you might test with files that happen to have even-length chunks and never notice.
|
||||
|
||||
**Consequences:** Genre filtering (which uses `recording_genres`) shows different results than the genre string displayed in the track list (which comes from `recordings.genre` via `track_metadata` VIEW).
|
||||
**Consequences:** Parsers following the misaligned chunk will read garbage as the next chunk ID. Some parsers handle this gracefully (try realignment), others crash or report corruption.
|
||||
|
||||
**Prevention:**
|
||||
1. **Always update both representations in the same transaction.** When the user sets genre to "Rock, Pop":
|
||||
- Update `recordings.genre` = "Rock, Pop"
|
||||
- Delete all `recording_genres` rows for this recording
|
||||
- Insert new `recording_genres` rows for "Rock" and "Pop" (via `ParseGenres()`)
|
||||
2. **Use `ParseGenres()` consistently** for both display and storage.
|
||||
3. **When writing to the audio file**, join the individual genre names with the format's conventional separator (`;` for Vorbis Comments multi-value, `,` for ID3v2 TCON frame).
|
||||
1. After writing each chunk's data, check if `chunk_data_length % 2 != 0`. If so, write one zero byte.
|
||||
2. Don't count this byte in the chunk's size field.
|
||||
3. DO count it when calculating the outer RIFF size (since it's part of the physical file).
|
||||
4. Test with an `id3 ` chunk that has odd-length data.
|
||||
|
||||
**Detection:** Edit genre, verify both the displayed genre string and the genre filter show consistent results.
|
||||
|
||||
**Phase:** Database update layer
|
||||
**Phase:** WAV writer
|
||||
|
||||
---
|
||||
|
||||
### P11: Database Update After File Write — Partial Failure Leaves Inconsistency
|
||||
### P11: Reading/Writing Library Asymmetry
|
||||
|
||||
**What goes wrong:** The tag edit flow is: (1) write new tags to temp file, (2) rename temp to original, (3) update DB entities, (4) update FTS5 index. If step 2 succeeds but step 3 fails (e.g., SQLite busy, constraint violation), the file on disk has new tags but the DB shows old values. The next scan will "fix" this by re-reading the file, but until then the UI shows stale data.
|
||||
**Severity:** MEDIUM — data loss or failure if libraries disagree on format
|
||||
**What goes wrong:** The reading path uses `dhowden/tag` for metadata extraction. The writing path uses different libraries: `bogem/id3v2` for MP3, `go-flac/go-flac` for FLAC, and will use custom code for OGG. If these libraries disagree on field encoding, round-trip testing will fail. For example, dhowden/tag may normalize certain fields that the writer preserves verbatim, or vice versa.
|
||||
|
||||
**Why it happens:** File writes and DB writes can't be in the same transaction (they're different systems). The rename is the point of no return for the file.
|
||||
**Why it happens:** No single Go library handles both reading and writing for all formats. Each library has its own interpretation of edge cases.
|
||||
|
||||
**Consequences:** UI shows old metadata for edited tracks. Search returns old values. User thinks the edit failed and tries again (potentially fine since the file is already correct).
|
||||
**Consequences:** Tags appear to be saved but read back differently. Or dhowden/tag can't parse what the writer produces.
|
||||
|
||||
**Prevention:**
|
||||
1. **DB update first approach:** Update the DB entities BEFORE writing the file. If DB update fails, don't write the file — clean rollback. If DB update succeeds but file write fails, revert the DB change. This makes the DB the "leader" and the file the "follower."
|
||||
2. **Alternative: Accept eventual consistency.** Write file, update DB, if DB fails log a warning and mark the file for re-scan. The scan pipeline already handles files-on-disk-differ-from-DB.
|
||||
3. **For batch edits:** Use a two-phase approach — first update all DBs in a transaction, then write all files. If any file write fails, the DB is already correct for the others. Report per-file errors to the user.
|
||||
4. **Recommendation:** Option 1 (DB first) is simpler and more correct. The file write is the expensive/risky step; the DB update is fast and transactional.
|
||||
1. **Round-trip testing is mandatory for every format.** Write tags → read back with dhowden/tag → assert all 8 fields match.
|
||||
2. For OGG: since we're writing the Vorbis Comment bytes ourselves, we control the exact format. Use the same field names dhowden/tag expects (TITLE, ARTIST, ALBUM, ALBUMARTIST, GENRE, DATE, TRACKNUMBER, DISCNUMBER, COMPOSER).
|
||||
3. For WAV: use `bogem/id3v2` (same as MP3) for the ID3v2 chunk content. The round-trip behavior should match MP3 tests.
|
||||
4. Watch for Vorbis Comment field name case sensitivity: field names are case-insensitive per spec, but dhowden/tag may return them in a specific case. Our writer should use UPPERCASE field names (Vorbis convention).
|
||||
|
||||
**Detection:** Kill the app mid-edit (during file write), restart, verify DB and file are consistent.
|
||||
|
||||
**Phase:** Tag write integration layer
|
||||
**Phase:** All writer implementations
|
||||
|
||||
---
|
||||
|
||||
### P12: `dhowden/tag` Is Read-Only — Need Separate Write Libraries Per Format
|
||||
### P12: AtomicWrite Disk Space for Large Files
|
||||
|
||||
**What goes wrong:** The existing `github.com/dhowden/tag` library is read-only. It extracts tags but cannot write them. Developers may assume the existing dependency can handle writes, waste time trying, then discover late that a separate library is needed.
|
||||
**Severity:** MEDIUM — write fails on low disk space
|
||||
**What goes wrong:** AtomicWrite creates a temp file alongside the original, writes the complete new file, then renames. For a 700MB WAV file, this requires 700MB of free disk space. If the disk is nearly full, the temp file write will fail partway through, and the deferred cleanup removes the partial temp file — no data loss, but the user gets an error.
|
||||
|
||||
**Why it happens:** `dhowden/tag` explicitly only supports reading. Its API has `ReadFrom()` but no `WriteTo()`.
|
||||
**Why it happens:** WAV files can be gigabytes (24-bit, 96kHz stereo recordings). OGG files are typically much smaller (compressed), but large FLAC→OGG conversions at high quality can still be hundreds of MB.
|
||||
|
||||
**Consequences:** Need to add 1-2 new dependencies for tag writing, each with different APIs and behaviors per format.
|
||||
**Consequences:** Write fails with "no space left on device." AtomicWrite correctly cleans up — no corruption. But the user can't save tags without freeing disk space.
|
||||
|
||||
**Prevention:**
|
||||
1. **MP3 (ID3v2):** Use `github.com/bogem/id3v2/v2` (also available as `github.com/n10v/id3v2/v2`). 359 stars, actively maintained, supports read+write for ID3v2.3 and v2.4, handles encoding correctly, supports picture frames. Pure Go.
|
||||
2. **FLAC:** Use `github.com/go-flac/flactag` or handle FLAC metadata blocks manually. FLAC's metadata format is simpler than ID3v2 (well-defined block structure). May need to write a thin wrapper that reads STREAMINFO + other blocks, modifies VORBIS_COMMENT block, and rewrites.
|
||||
3. **OGG Vorbis:** Use `github.com/go-flac/go-ogg` or a Vorbis Comment library. OGG wraps Vorbis Comments in OGG pages, which adds framing complexity.
|
||||
4. **WAV:** WAV tag support is minimal in practice. Defer WAV tag writing (not in v1.2 scope per PROJECT.md which lists MP3, FLAC, OGG only).
|
||||
5. **Keep `dhowden/tag` for reading.** Don't replace it — use it alongside the write libraries.
|
||||
1. **Pre-flight check:** Before starting the write, check available disk space. If free space < file size + some margin (10MB), return a clear error: "insufficient disk space for atomic write."
|
||||
2. Log the file size at INFO level when writing large files (>100MB) so users understand why it takes time.
|
||||
3. The existing `largeSizeThreshold` warning in `writeFlacTags` (500MB) should be applied to all format writers.
|
||||
4. For WAV specifically, where we're rewriting a potentially multi-GB file: consider whether the tag data can be appended at the end without rewriting the audio data. If the `id3 ` chunk is placed after `data`, and the RIFF header size is updated, this is theoretically possible — but tricky to get right and incompatible with the current AtomicWrite pattern.
|
||||
|
||||
**Phase:** Stack decision (before implementation begins)
|
||||
**Phase:** WAV writer, integration testing
|
||||
|
||||
---
|
||||
|
||||
### P13: OGG Multi-Stream Files (Chained and Multiplexed)
|
||||
|
||||
**Severity:** MEDIUM — silent data loss or crash on unusual files
|
||||
**What goes wrong:** OGG supports two types of multiplexing: **chaining** (sequential streams, like concatenated songs in internet radio recordings) and **grouping** (interleaved streams, like video + audio). A chained OGG file has multiple logical bitstreams end-to-end, each with its own BOS/EOS pages. If our writer assumes a single logical bitstream, it will either only modify the first stream's tags (losing all subsequent streams) or crash when it encounters unexpected BOS pages.
|
||||
|
||||
**Why it happens:** Most `.ogg` music files contain a single Vorbis stream. But files from internet radio captures, or OGG files with embedded lyrics/subtitles, may have multiple streams.
|
||||
|
||||
**Consequences:** Chained streams after the first are silently dropped. File plays only the first few seconds/minutes. Data loss that the user may not notice immediately.
|
||||
|
||||
**Prevention:**
|
||||
1. **Detect multi-stream files early:** Check for a second BOS page (after the first EOS). If found, either:
|
||||
- (a) Return an error: "multi-stream OGG files are not supported for tag editing"
|
||||
- (b) Only modify the first stream and pass through all subsequent streams byte-for-byte
|
||||
2. Option (a) is safer for v1.2.1. Option (b) is more user-friendly but requires more careful implementation.
|
||||
3. **Detect multiplexed streams:** If pages with different serial numbers appear interleaved, reject the file. YellowJacket is a music player — multiplexed OGG (video+audio) is out of scope.
|
||||
4. Test with a chained OGG file to verify graceful handling.
|
||||
|
||||
**Phase:** OGG writer (validation at file open time)
|
||||
|
||||
---
|
||||
|
||||
### P14: Round-Trip Data Loss — Fields Present in File But Not in Our Model
|
||||
|
||||
**Severity:** MEDIUM — data loss for power users
|
||||
**What goes wrong:** Our model has 8 editable fields + cover art. Vorbis Comments can contain arbitrary fields (LYRICS, COMMENT, PERFORMER, ORGANIZATION, REPLAYGAIN_*, etc.). When we rewrite the OGG file, if we rebuild the Vorbis Comment block from scratch using only our 8 fields, all other comment fields are lost. The same applies to WAV — an ID3v2 tag may have many frames beyond our 8 fields.
|
||||
|
||||
**Why it happens:** The simplest implementation is "build new comment block from scratch." This is what go-flac/flacvorbis does for FLAC — and it works for FLAC because the library preserves the comment block and we only replace specific fields.
|
||||
|
||||
**Consequences:** ReplayGain data lost (affects volume normalization in other players). Custom fields from other tools lost. Lyrics lost. Users who also use other tagging software will lose data.
|
||||
|
||||
**Prevention:**
|
||||
1. **For OGG:** Parse the existing Vorbis Comment block. Replace/add only the fields we're changing. Preserve all other comment entries verbatim. This is the same approach already used in `replaceVorbisComment()` for FLAC — port that logic.
|
||||
2. **For WAV (ID3v2):** The `bogem/id3v2` library already handles this — opening with `Parse: true` reads all existing frames, and writing only replaces/adds what we change.
|
||||
3. **Cover art special case:** When setting cover art in OGG, remove existing `METADATA_BLOCK_PICTURE` entries but preserve all other fields. When clearing cover art, only remove `METADATA_BLOCK_PICTURE`.
|
||||
4. Test: create a file with extra fields (e.g., REPLAYGAIN_TRACK_GAIN), edit one of our fields, verify the extra fields survive.
|
||||
|
||||
**Phase:** OGG writer, WAV writer
|
||||
|
||||
---
|
||||
|
||||
## Minor Pitfalls
|
||||
|
||||
Mistakes that cause minor issues, confusion, or suboptimal UX.
|
||||
Mistakes that cause edge-case bugs, poor UX, or unexpected behavior.
|
||||
|
||||
### P13: Cover Art Cache Invalidation After Embedded Art Change
|
||||
### P15: OGG Page Segmentation for Large Comment Headers
|
||||
|
||||
**What goes wrong:** Cover art is cached by content hash in `~/.local/share/yellowjacket/covers/`. If the user replaces embedded cover art, the old cached thumbnails (sm/md/lg) still exist and may be served from cache. The cover art hash changes (new image = new hash), so a new cache entry is created, but the `release_groups.cover_art_id` must be updated to point to the new `cover_art` record.
|
||||
**Severity:** LOW-MEDIUM — fails for large cover art
|
||||
**What goes wrong:** OGG pages have a max segment table of 255 entries, each representing up to 255 bytes, giving a max page data size of 65,025 bytes (~63.5KB). A comment header with embedded cover art can easily exceed this. The header packet must then span multiple pages, using the continued-packet mechanism (header type flag 0x01 on continuation pages). If the page segmentation code doesn't handle multi-page packets, writing large cover art will fail or produce corrupt files.
|
||||
|
||||
**Why it happens:** The cover art system is designed for initial extraction during scan. It doesn't expect art to change after initial import.
|
||||
**Why it happens:** Most comment headers fit in a single page. The multi-page case only triggers with cover art or very long field values.
|
||||
|
||||
**Prevention:**
|
||||
1. After writing new cover art to the file, extract it back, compute the new hash, create the new `cover_art` record, update `release_groups.cover_art_id`, generate new thumbnails.
|
||||
2. Delete old `cover_art` record and files only if no other release_group references them (same orphan cleanup as P9).
|
||||
3. Emit an event so the frontend refreshes cover art display (invalidate any cached cover art URLs).
|
||||
1. Implement proper packet-to-page segmentation: split packets into 255-byte segments, fill pages up to 255 segments each, set continuation flag on subsequent pages.
|
||||
2. Test with a comment header that's exactly 65,025 bytes (fits in one page), 65,026 bytes (requires two pages), and ~200KB (requires multiple pages).
|
||||
3. Consider implementing cover art writing as a second phase after text fields are working and tested.
|
||||
|
||||
**Phase:** Cover art write layer
|
||||
**Phase:** OGG writer (advanced)
|
||||
|
||||
---
|
||||
|
||||
### P14: Undo/Redo Expectations — Users Expect to Revert Tag Edits
|
||||
### P16: WAV Files >4GB (RF64 Format)
|
||||
|
||||
**What goes wrong:** User changes artist name, saves, realizes it was wrong, expects Ctrl+Z to work. But tag editing writes to the actual audio file — there's no undo buffer.
|
||||
**Severity:** LOW — rare in music player context
|
||||
**What goes wrong:** Standard WAV uses 32-bit chunk sizes, limiting files to ~4GB. WAV files larger than 4GB use the RF64 extension (magic `RF64` instead of `RIFF`, with a `ds64` chunk for 64-bit sizes). If we encounter an RF64 file and treat it as standard WAV, we'll misparse the chunk sizes and produce a corrupt file.
|
||||
|
||||
**Why it happens:** File writes are destructive. The temp-file-rename approach ensures atomicity but not reversibility.
|
||||
**Why it happens:** RF64 is common in professional audio (long multi-channel recordings). Most music collections won't have these, but a user with high-resolution recordings might.
|
||||
|
||||
**Consequences:** File corruption for files >4GB. Audio data shifted due to wrong chunk size calculation.
|
||||
|
||||
**Prevention:**
|
||||
1. **For v1.2: Don't implement undo.** It's complex (would need to store original tag values per-edit) and users of tag editors generally don't expect undo.
|
||||
2. **Show a confirmation dialog before writing**, especially for batch edits. "You are about to modify 47 files. This cannot be undone."
|
||||
3. **Log what changed.** Write structured log entries like `"tag edit: file=/path/to/song.mp3, field=artist, old=Beatles, new=The Beatles"`. This gives users a recovery path (manual).
|
||||
4. **Future milestone consideration:** Backup original files before edit (copy to `.yj-backup/` directory). Add a "restore original" option.
|
||||
1. **Check for RF64 magic** (`RF64` at offset 0 instead of `RIFF`). If found, return an error: "RF64 WAV files are not supported for tag editing."
|
||||
2. This is acceptable for v1.2.1 — RF64 support can be added later if users report needing it.
|
||||
3. Also check: if the calculated file size would exceed 4GB after adding/expanding the ID3v2 chunk, warn that the file may be problematic.
|
||||
|
||||
**Phase:** UX design
|
||||
**Phase:** WAV writer (validation at file open time)
|
||||
|
||||
---
|
||||
|
||||
### P15: Track Number and Disc Number Edge Cases
|
||||
### P17: Test Fixture Creation for OGG and WAV
|
||||
|
||||
**What goes wrong:** Track number is stored as `sql.NullInt64` in the DB and as `int` in `TrackMetadata`. User enters "1/12" in the track number field (common display format). If parsed as a raw int, this fails. If split on `/`, `TotalTracks` must also be stored. The existing `toNullInt64()` treats 0 as NULL, so track 0 is impossible to store (rare but exists in some compilations).
|
||||
**Severity:** LOW-MEDIUM — blocks test coverage
|
||||
**What goes wrong:** The existing FLAC and MP3 tests create minimal valid files programmatically (see `makeMinimalFLAC` and `createTestMP3`). OGG Vorbis files are harder to create programmatically because you need valid identification, comment, and setup header packets. WAV files are simpler but still require correct RIFF structure.
|
||||
|
||||
**Consequences:** Track numbers display incorrectly or can't be set to certain values.
|
||||
**Why it happens:** OGG Vorbis requires three header packets where the setup header contains Vorbis codebook data. You can't easily generate a valid setup header from scratch without a Vorbis encoder.
|
||||
|
||||
**Prevention:**
|
||||
1. Parse "N/M" format: split on `/`, store track number and total separately.
|
||||
2. Validate inputs: track number must be positive integer (or blank for null).
|
||||
3. Consider whether `toNullInt64()` treating 0 as NULL is correct for the edit case. For display it's fine, but for editing, the user might explicitly set track number to 0. Probably not worth changing for v1.2.
|
||||
**Prevention for OGG:**
|
||||
1. **Embed a minimal OGG fixture as `//go:embed`** in the test file. Create it once with an encoder (e.g., `ffmpeg -f lavfi -i "sine=frequency=440:duration=0.1" -c:a libvorbis -q:a 0 minimal.ogg`). ~5KB file.
|
||||
2. Copy this fixture to a temp directory in each test, then modify it.
|
||||
3. Alternatively, use `jfreymuth/oggvorbis` (already an indirect dep) to understand the page structure and craft test files with known structure.
|
||||
|
||||
**Phase:** Frontend input validation + backend write layer
|
||||
**Prevention for WAV:**
|
||||
1. **Generate minimal WAV programmatically** — it's much simpler than OGG:
|
||||
- RIFF header (12 bytes): "RIFF" + size + "WAVE"
|
||||
- fmt chunk (24 bytes): "fmt " + 16 + PCM format data
|
||||
- data chunk (8+ bytes): "data" + size + silence samples
|
||||
This is similar to `makeMinimalFLAC` in complexity.
|
||||
2. Generate fixtures with and without existing ID3v2 chunks to test both "add new" and "replace existing" paths.
|
||||
|
||||
**Phase:** Test infrastructure (before format writers)
|
||||
|
||||
---
|
||||
|
||||
### P16: Multiple Audio Files Sharing the Same Recording (1:1 Assumption)
|
||||
### P18: Empty/Missing Tags vs Zero-Length Strings
|
||||
|
||||
**What goes wrong:** The scan pipeline creates a new `recordings` row for every audio file (see `processMetadata()` at `library.go:1178`). This means the relationship is effectively 1:1 (each audio_file has its own recording). But the schema allows N:1 (multiple audio_files can share a recording_id). If a future change or manual DB edit creates shared recordings, editing one track's metadata would affect the other track sharing that recording.
|
||||
**Severity:** LOW — cosmetic but confusing
|
||||
**What goes wrong:** When a user clears a field (sets it to empty string), should we write an empty comment entry (`TITLE=`) or omit the field entirely? Different readers handle these differently. dhowden/tag returns empty string for both missing and empty fields, so the distinction is invisible on read. But other tools may show "unknown" for missing fields vs blank for empty fields.
|
||||
|
||||
**Why it happens:** The schema was designed for MusicBrainz-style data where multiple releases of the same recording share a recording ID. The scan pipeline doesn't implement this sharing, but the schema allows it.
|
||||
**Why it happens:** The Vorbis Comment spec says nothing about whether empty values are allowed (they are — any UTF-8 string including empty is valid).
|
||||
|
||||
**Prevention:**
|
||||
1. **Before editing a recording, check how many audio_files reference it.** If more than one, create a new recording for this audio_file (fork the entity).
|
||||
2. This is already handled by P4's "always create new" approach, but worth calling out as a specific guard.
|
||||
1. **Match FLAC behavior:** The existing `replaceVorbisComment` function writes the new value regardless of whether it's empty. An empty string results in `TITLE=`. This is fine.
|
||||
2. For OGG, use the same approach: write `FIELD=value` for all fields in the diff map, whether value is empty or not.
|
||||
3. For WAV ID3v2: an empty string text frame is valid. The bogem/id3v2 library handles this correctly.
|
||||
|
||||
**Phase:** Database update layer
|
||||
**Phase:** All writers
|
||||
|
||||
---
|
||||
|
||||
### P17: Frontend Store Refresh After Tag Edit
|
||||
### P19: Unicode in Vorbis Comment Field Values
|
||||
|
||||
**What goes wrong:** After a tag edit updates the DB, the frontend `libraryStore` still holds the old cached data (tracks, albums, artists, genres). Without a refresh, the UI shows stale values until the user navigates away and back, or triggers a full reload.
|
||||
|
||||
**Why it happens:** The `libraryStore.eagerFetch()` loads all data at startup. There's no mechanism for partial updates — the store either shows cached data or refetches everything.
|
||||
**Severity:** LOW — most data is ASCII but international users need this
|
||||
**What goes wrong:** Vorbis Comment values MUST be valid UTF-8. Go strings are inherently UTF-8, so this is mostly automatic. However, if the user pastes text from a non-UTF-8 source (e.g., Windows-1252), the bytes won't be valid UTF-8 and could cause downstream parsing issues.
|
||||
|
||||
**Prevention:**
|
||||
1. **Emit a `TagsUpdated` event** from the backend after successful tag edit, with the list of affected file paths.
|
||||
2. The frontend store listens for this event and either:
|
||||
- (a) Refetches the full data (simple but expensive for large libraries), or
|
||||
- (b) Patches the affected rows in-place (more complex but instant)
|
||||
3. **Recommendation for v1.2:** Option (a) — full refetch. The existing `eagerFetch()` path is proven. Optimize to partial updates in a future milestone if performance is an issue.
|
||||
4. Also update: search results (refetch if search is active), queue track metadata (emit `QueueTracksModified`), now-playing display (emit `TrackChanged` if the edited track is playing).
|
||||
1. Validate that all string values are valid UTF-8 before writing: `utf8.ValidString(value)`.
|
||||
2. This is unlikely to be an issue since Wails serializes strings as JSON (which mandates UTF-8), but a defensive check is cheap.
|
||||
|
||||
**Phase:** Frontend integration (last phase)
|
||||
**Phase:** All writers (validation layer)
|
||||
|
||||
---
|
||||
|
||||
### P20: Existing BWF (Broadcast Wave Format) Chunks in WAV
|
||||
|
||||
**Severity:** LOW — professional users may care
|
||||
**What goes wrong:** Some WAV files contain `bext` (Broadcast Extension) chunks with professional metadata (origination date, time reference, loudness info). Our writer should not delete or corrupt these chunks.
|
||||
|
||||
**Prevention:**
|
||||
1. **Preserve all unrecognized chunks verbatim.** When rewriting the WAV file, copy all chunks we don't modify byte-for-byte to the output.
|
||||
2. Only modify the `id3 ` chunk (add, replace, or update). Leave `fmt `, `data`, `bext`, `cue `, `smpl`, and all other chunks untouched.
|
||||
3. The full-rewrite approach: iterate chunks in the source file, copy each to the output, replacing `id3 ` with our new version (or appending it at the end if it didn't exist).
|
||||
|
||||
**Phase:** WAV writer
|
||||
|
||||
---
|
||||
|
||||
## Phase-Specific Warnings
|
||||
|
||||
| Phase Topic | Likely Pitfall | Mitigation |
|
||||
|-------------|---------------|------------|
|
||||
| Schema migration | P3: FTS5 contentless can't delete | Migrate to `contentless_delete=1` first |
|
||||
| File write layer | P1: FLAC full rewrite, P6: temp file same dir | Write-to-temp-then-rename in same directory |
|
||||
| File write layer | P2: Currently playing file | Check player state before write, stop if needed |
|
||||
| Tag library selection | P12: dhowden/tag is read-only | Use bogem/id3v2 for MP3, format-specific libs for FLAC/OGG |
|
||||
| DB update design | P4: Shared entities, P9: Orphan cleanup | Always create new entities, clean up orphans per-edit |
|
||||
| DB update design | P10: Genre dual representation | Update both recordings.genre and recording_genres atomically |
|
||||
| Write + DB integration | P5: Scan race condition | Pause scan during edit, or mutual exclusion |
|
||||
| Write + DB integration | P11: Partial failure | DB update first, then file write |
|
||||
| Cover art writes | P8: Size/format compat, P13: Cache invalidation | Resize before embed, invalidate cache after write |
|
||||
| Encoding | P7: ID3v2 Latin-1 vs UTF-8 | Use UTF-16 for v2.3, UTF-8 for v2.4 |
|
||||
| Frontend | P17: Stale cache after edit | Emit event, full refetch |
|
||||
| UX design | P14: No undo for file writes | Confirmation dialog, structured logging |
|
||||
| Phase Topic | Likely Pitfall | Severity | Mitigation |
|
||||
|------------|---------------|----------|------------|
|
||||
| OGG page infrastructure | P1 (CRC32), P2 (seq numbers) | CRITICAL | Port CRC from oggvorbis/crc.go; full-stream rewrite |
|
||||
| OGG Vorbis comment writer | P4 (three headers), P7 (framing bit), P8 (prefix) | HIGH | Follow Vorbis I spec exactly; round-trip test |
|
||||
| OGG cover art | P9 (base64 PICTURE), P15 (large pages) | MEDIUM-HIGH | METADATA_BLOCK_PICTURE format; multi-page support |
|
||||
| OGG edge cases | P13 (multi-stream), P3 (granule positions) | MEDIUM | Detect and reject multi-stream; preserve granule positions |
|
||||
| WAV writer core | P5 (ID3v2 vs RIFF INFO), P6 (RIFF size) | CRITICAL | Write ID3v2; careful size bookkeeping |
|
||||
| WAV chunk handling | P10 (alignment), P16 (RF64), P20 (BWF) | MEDIUM | Pad to even; detect RF64; preserve all chunks |
|
||||
| Integration | P11 (read/write asymmetry), P14 (data loss) | MEDIUM | Round-trip tests; preserve unknown fields |
|
||||
| Large files | P12 (disk space) | MEDIUM | Pre-flight size check; AtomicWrite handles crash safety |
|
||||
| Testing | P17 (fixtures) | LOW-MEDIUM | Embed OGG fixture; generate WAV programmatically |
|
||||
|
||||
## Ordering Implications
|
||||
## Implementation Order Recommendation
|
||||
|
||||
The pitfalls strongly suggest this phase ordering:
|
||||
Based on pitfall severity and dependency chain:
|
||||
|
||||
1. **FTS5 migration first** (P3) — enables all subsequent DB updates to be clean
|
||||
2. **File write layer** (P1, P2, P6) — the atomic write-to-temp-rename mechanism, independent of DB
|
||||
3. **Tag library integration** (P7, P8, P12) — per-format write support using new dependencies
|
||||
4. **DB update design** (P4, P9, P10, P11, P16) — entity creation, orphan cleanup, genre sync
|
||||
5. **Scan pipeline integration** (P5) — mutual exclusion between edit and scan
|
||||
6. **Frontend** (P14, P15, P17) — UI, events, cache refresh
|
||||
1. **OGG page infrastructure** (P1, P2, P3) — must be correct before anything else works
|
||||
2. **OGG comment writer** (P7, P8, P4, P14) — build on page infrastructure
|
||||
3. **WAV writer** (P5, P6, P10, P20) — independent of OGG, can parallelize
|
||||
4. **OGG cover art** (P9, P15) — can be deferred if text fields work
|
||||
5. **Edge case handling** (P13, P16, P12) — validation and error reporting
|
||||
6. **Round-trip testing** (P11, P17) — continuous throughout, but formalize at end
|
||||
|
||||
## Key Decision Points
|
||||
|
||||
### OGG: Full Rewrite vs Surgical Edit
|
||||
**Recommendation: Full rewrite.** The approach of "read all pages → replace comment packet → rewrite all pages" is simpler, more correct, and matches the FLAC precedent. The performance cost is acceptable because OGG files are compressed (typically 3-10MB for a song). A surgical edit (only rewriting comment pages and adjusting subsequent pages) saves I/O but dramatically increases complexity for CRC, sequence numbers, and page boundary management.
|
||||
|
||||
### WAV: Where to Place ID3v2 Chunk
|
||||
**Recommendation: After the `data` chunk.** This ensures naive parsers that stop reading after `data` still play the file. More sophisticated parsers (like dhowden/tag) scan the entire RIFF structure and will find the `id3 ` chunk wherever it is.
|
||||
|
||||
### Cover Art in OGG: v1.2.1 or Defer?
|
||||
**Recommendation: Implement text fields first, add cover art as stretch goal.** Cover art in OGG (P9, P15) adds significant complexity (base64 encoding, multi-page packets, FLAC PICTURE structure). Text-only tag editing is valuable on its own. If cover art doesn't make v1.2.1, it's a clean addition in a follow-up.
|
||||
|
||||
## Sources
|
||||
|
||||
- SQLite FTS5 documentation: contentless tables section (sqlite.org/fts5.html#contentless_tables) — HIGH confidence
|
||||
- SQLite FTS5 contentless_delete: sqlite.org/fts5.html#contentless_delete_tables — HIGH confidence
|
||||
- YellowJacket codebase analysis: search.go, library.go, player.go, schema files — HIGH confidence
|
||||
- FLAC format spec: metadata block structure, PICTURE block format — HIGH confidence (well-established spec)
|
||||
- ID3v2.3/2.4 spec: encoding requirements for text frames — HIGH confidence
|
||||
- `bogem/id3v2` GitHub (n10v/id3v2): read+write ID3v2 library, 359 stars — MEDIUM confidence (verified repo exists and has write support)
|
||||
- `dhowden/tag` API: read-only confirmed from codebase usage — HIGH confidence
|
||||
- OGG Framing Specification: https://xiph.org/ogg/doc/framing.html (HIGH confidence — canonical spec)
|
||||
- RFC 3533 — The Ogg Encapsulation Format: https://www.rfc-editor.org/rfc/rfc3533 (HIGH confidence — IETF RFC)
|
||||
- Vorbis I Comment Spec: https://xiph.org/vorbis/doc/v-comment.html (HIGH confidence — canonical spec)
|
||||
- OGG Opus Mapping: https://wiki.xiph.org/OggOpus (HIGH confidence — Xiph wiki)
|
||||
- jfreymuth/oggvorbis source (crc.go, ogg.go): https://github.com/jfreymuth/oggvorbis (HIGH confidence — direct code review)
|
||||
- dhowden/tag source (ogg.go): https://github.com/dhowden/tag/blob/master/ogg.go (HIGH confidence — direct code review)
|
||||
- RIFF tag reference: https://exiftool.org/TagNames/RIFF.html (MEDIUM confidence — ExifTool documentation)
|
||||
- YellowJacket codebase analysis: tagwriter/, fileutil/, metadata/ packages (HIGH confidence — direct code review)
|
||||
|
||||
Reference in New Issue
Block a user