fix(v2.3): apply unsynchronisation at tag level per §5 - #666
Merged
Conversation
ID3v2.2 §6.1 and ID3v2.3 §5 define unsynchronisation as a TAG-level operation: when the flag is set, the entire concatenated frame stream (headers plus bodies) is unsynchronised once and the reader reverses it before parsing frames. Frame bodies must NOT carry the unsync byte-stuffing individually — sizes declared in frame headers refer to raw (pre-unsync) body lengths. Previously the writer: 1. Unsynchronised each v2.3 frame body individually. 2. Set the tag-level unsync flag anyway. This was internally consistent because the decoder also applied unsync per-frame (due to a separate bug where `version === 4` compared an array), but it violated the spec: the frame size fields declared unsynched lengths, frame headers were never unsynchronised even though they sat inside a stream the tag-level flag claimed was unsynched, and any compliant v2.3 reader that un-unsyncs at tag level would see misaligned frames and corrupt data. Changes: - encode(): for v2.3/v2.2, write frame bodies raw and apply `unsynch()` once to the concatenated frame stream before the tag-level flag is set. For v2.4, preserve per-frame unsync. - decode(): when reading v2.3/v2.2 with the tag-level flag set, un-unsynchronise the whole post-header region before frame parsing; track the post-synch length separately from the unsynched size declared in the tag header. - decodeFrame(): drop the now-dead tag-level fallback; only the v2.4 per-frame flag governs per-frame un-unsync. The new test asserts the GEOB frame size declared on disk equals the raw body length (49 bytes), not the unsynched length (51). Fails on the old code, passes on the new.
joust
force-pushed
the
fix/v23-tag-level-unsync
branch
from
April 15, 2026 10:18
64e05d8 to
6222ef5
Compare
2 tasks
joust
marked this pull request as ready for review
June 22, 2026 13:57
eidoriantan
approved these changes
Jun 22, 2026
Owner
|
Hi @joust , There are some changes merged into Thanks! |
Owner
|
Actually, seems like these are just simple conflicts. I'll handle this. Thanks |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ID3v2.2 §6.1 and ID3v2.3 §5 define unsynchronisation as a tag-level operation: when the flag is set, the entire concatenated frame stream is unsynchronised once and the reader reverses it before parsing. Frame sizes declared in headers refer to raw (pre-unsync) lengths.
The previous implementation unsynchronised each v2.3 frame body individually AND set the tag-level flag. Internally consistent (self-roundtrippable), but:
Changes
encode(): for v2.3/v2.2, write frame bodies raw and applyunsynch()once to the concatenated frame stream before setting the tag-level flag. v2.4 keeps per-frame unsync.decode(): when reading v2.3/v2.2 with tag-level flag set, un-unsynchronise the whole post-header region before frame parsing; track the post-synch length separately from the declared size.decodeFrame(): drop the dead tag-level fallback; only the v2.4 per-frame flag governs per-frame un-unsync. This subsumes the one-line fix in PR fix(v2.4): honor per-frame unsynchronisation flag #665 (version === 4→version[0] === 4).Relationship to #665
This PR's
decodeFramerefactor supersedes the one-line fix in #665. The regression test from #665 (per-frame unsync with tag-level bit cleared, v2.4) has been mirrored into this PR's test file as a companion assertion, so that merging this PR first does not lose that coverage.If both PRs are merged, #666 should go first; #665's one-line change then becomes a no-op and the PR can be closed.
Backward compatibility note
Tags written by earlier mp3tag.js releases with the old per-frame-unsync-plus-tag-level-bit layout will NOT be correctly read by this version. That format was non-compliant and unreadable by every other v2.3 parser anyway; any tag that can be read by mutagen, TagLib, eyeD3, Kid3, or Picard will continue to work.
Test plan
npm test— 80 passing (78 → 80: two new tests)