fix(v2.4): honor per-frame unsynchronisation flag#665
Draft
joust wants to merge 1 commit into
Draft
Conversation
ID3v2.4 §4.1.2 moves unsynchronisation from a tag-level flag (as in
v2.3 §5) to a per-frame flag. The tag-level bit in v2.4 is only a
hint meaning "at least one frame is unsynchronised"; the actual
decision of whether to un-unsynchronise a given frame must consult
that frame's own flag byte.
The decoder had the right intent but an incorrect check:
if (version === 4) unsynchedData = frame.flags.unsynchronisation
`version` at this scope is the [major, revision] array returned from
the ID3 header, so `version === 4` is always false. The branch was
dead code and the per-frame unsync flag was silently ignored. Tags
produced by standards-compliant v2.4 writers — where unsync is set
only at the frame level — would be read as raw unsynchronised bytes.
The previous behavior "worked" only when the tag-level unsync bit
was also set, which mp3tag.js itself does (see the separate v2.3
tag-level unsync issue).
The new test writes a v2.4 tag with unsync enabled, then clears the
tag-level unsync bit in the buffer to simulate a conforming writer,
and verifies the reader still decodes correctly via the per-frame
flag. Fails on the old code, passes on the new.
4 tasks
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
1 participant
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.4 §4.1.2 makes unsynchronisation a per-frame flag; the tag-level bit in v2.4 is only a hint. The decoder had the right intent but compared
version === 4whereversionis the[major, revision]array — always false. The branch was dead code; per-frame unsync was silently ignored.Standards-compliant v2.4 tags (unsync only at frame level, no tag-level bit) could not be decoded. The previous behavior happened to work only because mp3tag.js also sets the tag-level bit when writing v2.4 — see the related v2.3 tag-level unsync PR.
Changes
src/id3v2/index.mjs:version === 4→version[0] === 4, plus spec-citing commenttest/id3v2/index.cjs: new regression test — writes v2.4 GEOB with 0xFF bytes and unsync, clears the tag-level bit in the buffer, then asserts the reader still decodes the frame correctly via the per-frame flagRelationship to #666
PR #666 (v2.3 tag-level unsync) touches the same line and restructures
decodeFrameto drop the tag-level fallback entirely. The one-line fix in this PR is effectively included in #666 as part of that restructure, and the regression test here has been mirrored into #666 so that merging #666 first does not lose the coverage.Merge options:
version[0] === 4either way).Test plan
npm test— 79 passing (78 → 79)version === 4), passes on new (version[0] === 4)