From 7c017e2766486aedd97edfc192578b542404908b Mon Sep 17 00:00:00 2001 From: Jan Svabenik Date: Wed, 27 May 2026 09:23:17 +0200 Subject: [PATCH] fix(rating): preserve audio data on Set(); add unit tests The previous implementation used tag.WriteTo() which only emits the ID3 tag (~311 bytes), leaving audio data behind. Files were truncated to just the tag on every rating write. New strategy: 1. Parse the 10-byte ID3v2 header to find the audio start offset. 2. Encode the new tag into an in-memory buffer via WriteTo. 3. Write tag + original audio into a temp file. 4. Try atomic os.Rename (works when Winamp does not hold the file). 5. Fall back to direct O_WRONLY|O_TRUNC write (works while Winamp plays, because Winamp opens with FILE_SHARE_WRITE on Windows). Tests cover: POPM<->stars mapping, id3v2AudioStart, full round-trip (1-5 stars + unrate) with audio-integrity check, and error cases. Co-Authored-By: Claude Sonnet 4.6 --- internal/rating/rating.go | 116 ++++++++++++++++---- internal/rating/rating_test.go | 186 +++++++++++++++++++++++++++++++++ 2 files changed, 283 insertions(+), 19 deletions(-) create mode 100644 internal/rating/rating_test.go diff --git a/internal/rating/rating.go b/internal/rating/rating.go index a7b17b2..6b75b68 100644 --- a/internal/rating/rating.go +++ b/internal/rating/rating.go @@ -15,6 +15,7 @@ package rating import ( "bytes" "fmt" + "io" "math/big" "os" "path/filepath" @@ -50,12 +51,15 @@ func Get(path string) (int, error) { // Set writes a 0–5 star rating into the POPM frame of path. // stars=0 removes any existing POPM frame (unrated). // -// bogem/id3v2's Save() writes a temp file and renames it over the original, -// which Windows denies when another process (Winamp) holds the file open -// without FILE_SHARE_DELETE. Instead we use WriteTo to stream the modified -// tag+audio into an in-memory buffer, close our read handle, then overwrite -// the original file in-place — which succeeds because Winamp opens files -// with FILE_SHARE_WRITE. +// Strategy: bogem/id3v2's Save() creates a temp file and renames it over +// the original. The rename fails when Winamp holds the file open (Windows +// denies rename without FILE_SHARE_DELETE). Instead we: +// 1. Find where the audio data starts (parse the ID3v2 header size). +// 2. Write the new ID3 tag to an in-memory buffer via WriteTo. +// 3. Build the complete new file (tag + audio) in a temp file. +// 4. Try rename — works when the file is not currently playing. +// 5. If rename fails, stream the temp file directly into the original file +// (requires Winamp to have opened with FILE_SHARE_WRITE, which it does). func Set(path string, stars int) error { if !isMP3(path) { return fmt.Errorf("rating.Set: not an MP3 file: %s", filepath.Base(path)) @@ -64,11 +68,17 @@ func Set(path string, stars int) error { return fmt.Errorf("rating.Set: stars must be 0–5, got %d", stars) } + // ── Step 1: locate audio data in the original file ──────────────────────── + audioStart, err := id3v2AudioStart(path) + if err != nil { + return fmt.Errorf("rating.Set: parse header: %w", err) + } + + // ── Step 2: encode new tag into memory ──────────────────────────────────── tag, err := id3.Open(path, id3.Options{Parse: true}) if err != nil { return fmt.Errorf("rating.Set: open: %w", err) } - tag.DeleteFrames("POPM") if stars > 0 { tag.AddFrame("POPM", id3.PopularimeterFrame{ @@ -77,27 +87,95 @@ func Set(path string, stars int) error { Counter: big.NewInt(0), }) } - - // Stream modified tag + audio into memory buffer while file is still open. - var buf bytes.Buffer - if _, err := tag.WriteTo(&buf); err != nil { + var tagBuf bytes.Buffer + if _, err := tag.WriteTo(&tagBuf); err != nil { tag.Close() - return fmt.Errorf("rating.Set: encode: %w", err) + return fmt.Errorf("rating.Set: encode tag: %w", err) } - tag.Close() // release read handle before we open for writing + tag.Close() // release read handle on original file - // Overwrite the original file in-place (no rename needed). - f, err := os.OpenFile(path, os.O_WRONLY|os.O_TRUNC, 0644) + // ── Step 3: write tag + audio to a temp file ────────────────────────────── + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, ".rating-*.tmp") if err != nil { - return fmt.Errorf("rating.Set: open for write: %w", err) + return fmt.Errorf("rating.Set: create temp: %w", err) } - defer f.Close() - if _, err := f.Write(buf.Bytes()); err != nil { - return fmt.Errorf("rating.Set: write: %w", err) + tmpPath := tmp.Name() + defer os.Remove(tmpPath) // clean up on any exit path + + if _, err := tmp.Write(tagBuf.Bytes()); err != nil { + tmp.Close() + return fmt.Errorf("rating.Set: write tag to temp: %w", err) + } + + // Copy audio data from original file into the temp file. + src, err := os.Open(path) + if err != nil { + tmp.Close() + return fmt.Errorf("rating.Set: open src: %w", err) + } + if _, err := src.Seek(audioStart, io.SeekStart); err != nil { + src.Close() + tmp.Close() + return fmt.Errorf("rating.Set: seek audio: %w", err) + } + if _, err := io.Copy(tmp, src); err != nil { + src.Close() + tmp.Close() + return fmt.Errorf("rating.Set: copy audio: %w", err) + } + src.Close() + if err := tmp.Close(); err != nil { + return fmt.Errorf("rating.Set: close temp: %w", err) + } + + // ── Step 4: try atomic rename (works when file is not open by Winamp) ───── + if err := os.Rename(tmpPath, path); err == nil { + return nil + } + + // ── Step 5: rename failed — stream temp into the original file directly ─── + // This works because Winamp opens MP3s with FILE_SHARE_WRITE. + tmpFile, err := os.Open(tmpPath) + if err != nil { + return fmt.Errorf("rating.Set: open temp for streaming: %w", err) + } + defer tmpFile.Close() + + dst, err := os.OpenFile(path, os.O_WRONLY|os.O_TRUNC, 0644) + if err != nil { + return fmt.Errorf("rating.Set: open dst: %w", err) + } + defer dst.Close() + + if _, err := io.Copy(dst, tmpFile); err != nil { + return fmt.Errorf("rating.Set: stream to dst: %w", err) } return nil } +// id3v2AudioStart reads the 10-byte ID3v2 header and returns the byte offset +// where audio data begins (i.e. the end of the tag). Returns 0 if no ID3v2 +// tag is present. +func id3v2AudioStart(path string) (int64, error) { + f, err := os.Open(path) + if err != nil { + return 0, err + } + defer f.Close() + + var hdr [10]byte + if _, err := io.ReadFull(f, hdr[:]); err != nil { + return 0, nil // file too short — no tag + } + if string(hdr[:3]) != "ID3" { + return 0, nil // no ID3v2 tag + } + // Bytes 6–9 are a synchsafe integer (7 bits per byte). + size := int64(hdr[6])<<21 | int64(hdr[7])<<14 | int64(hdr[8])<<7 | int64(hdr[9]) + return 10 + size, nil +} + func isMP3(path string) bool { return strings.EqualFold(filepath.Ext(path), ".mp3") } diff --git a/internal/rating/rating_test.go b/internal/rating/rating_test.go new file mode 100644 index 0000000..cf67a90 --- /dev/null +++ b/internal/rating/rating_test.go @@ -0,0 +1,186 @@ +package rating + +import ( + "bytes" + "os" + "testing" +) + +// buildMinimalMP3 returns a minimal valid MP3: a 10-byte ID3v2.3 header with +// no frames (tag size = 0) followed by 4096 bytes of fake audio data (0xFF 0xFB +// sync word repeated, which is close enough for our purposes). +func buildMinimalMP3() []byte { + var buf bytes.Buffer + // ID3v2.3 header: "ID3" + version 2.3.0 + flags 0 + synchsafe size 0 + buf.Write([]byte{'I', 'D', '3', 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}) + // Fake audio data + audio := make([]byte, 4096) + for i := range audio { + audio[i] = byte(i & 0xFF) + } + buf.Write(audio) + return buf.Bytes() +} + +func writeTempMP3(t *testing.T, data []byte) string { + t.Helper() + f, err := os.CreateTemp(t.TempDir(), "test-*.mp3") + if err != nil { + t.Fatalf("create temp: %v", err) + } + if _, err := f.Write(data); err != nil { + t.Fatalf("write temp: %v", err) + } + f.Close() + return f.Name() +} + +func TestPopmToStars(t *testing.T) { + cases := []struct{ in uint8; want int }{ + {0, 0}, {1, 1}, {31, 1}, {32, 2}, {95, 2}, + {96, 3}, {127, 3}, {128, 3}, {159, 3}, + {160, 4}, {195, 4}, {196, 4}, {223, 4}, + {224, 5}, {255, 5}, + } + for _, c := range cases { + got := popmToStars(c.in) + if got != c.want { + t.Errorf("popmToStars(%d) = %d, want %d", c.in, got, c.want) + } + } +} + +func TestStarsToPOPM(t *testing.T) { + want := [6]uint8{0, 1, 64, 128, 196, 255} + for i, w := range want { + got := starsToPOPM(i) + if got != w { + t.Errorf("starsToPOPM(%d) = %d, want %d", i, got, w) + } + } +} + +func TestId3v2AudioStart_noTag(t *testing.T) { + // File with no ID3 header + f, err := os.CreateTemp(t.TempDir(), "noTag-*.mp3") + if err != nil { + t.Fatal(err) + } + f.Write([]byte{0xFF, 0xFB, 0x90, 0x00}) // MP3 sync + f.Close() + + start, err := id3v2AudioStart(f.Name()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if start != 0 { + t.Errorf("got audioStart=%d, want 0", start) + } +} + +func TestId3v2AudioStart_withHeader(t *testing.T) { + // Header with tag size 0 → audioStart should be 10 + data := buildMinimalMP3() + path := writeTempMP3(t, data) + + start, err := id3v2AudioStart(path) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if start != 10 { + t.Errorf("got audioStart=%d, want 10", start) + } +} + +func TestSetAndGet_roundtrip(t *testing.T) { + original := buildMinimalMP3() + path := writeTempMP3(t, original) + + // File should start with no rating + r, err := Get(path) + if err != nil { + t.Fatalf("Get before Set: %v", err) + } + if r != 0 { + t.Errorf("expected 0 before Set, got %d", r) + } + + // Set each star value and read back + for stars := 1; stars <= 5; stars++ { + if err := Set(path, stars); err != nil { + t.Fatalf("Set(%d): %v", stars, err) + } + + // File must not be truncated — must be larger than tag-only (> original size) + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat after Set(%d): %v", stars, err) + } + if info.Size() < int64(len(original)) { + t.Errorf("Set(%d) shrunk file: got %d bytes, original was %d", + stars, info.Size(), len(original)) + } + + got, err := Get(path) + if err != nil { + t.Fatalf("Get after Set(%d): %v", stars, err) + } + if got != stars { + t.Errorf("Get after Set(%d) = %d, want %d", stars, got, stars) + } + } + + // Unrate (stars=0) must remove POPM + if err := Set(path, 0); err != nil { + t.Fatalf("Set(0): %v", err) + } + got, err := Get(path) + if err != nil { + t.Fatalf("Get after Set(0): %v", err) + } + if got != 0 { + t.Errorf("Get after Set(0) = %d, want 0", got) + } + + // Audio data must be intact after all the Set calls + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + audioStart, _ := id3v2AudioStart(path) + if audioStart >= int64(len(data)) { + t.Fatalf("audioStart %d >= fileSize %d", audioStart, len(data)) + } + got4096 := data[audioStart:] + orig4096 := original[10:] // original audio starts at byte 10 (tag size 0) + if len(got4096) < len(orig4096) { + t.Errorf("audio data truncated: got %d bytes, want %d", len(got4096), len(orig4096)) + } else { + // Last 4096 bytes should match original audio + tail := got4096[len(got4096)-len(orig4096):] + if !bytes.Equal(tail, orig4096) { + t.Error("audio data corrupted after rating Set calls") + } + } +} + +func TestSet_invalidStars(t *testing.T) { + path := writeTempMP3(t, buildMinimalMP3()) + if err := Set(path, 6); err == nil { + t.Error("Set(6) should return error") + } + if err := Set(path, -1); err == nil { + t.Error("Set(-1) should return error") + } +} + +func TestSet_notMP3(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "test-*.flac") + if err != nil { + t.Fatal(err) + } + f.Close() + if err := Set(f.Name(), 3); err == nil { + t.Error("Set on non-MP3 should return error") + } +}