ID3: Fix v2.4 extended header handling - Failure to extract tags #12
+42
−5
Conversation
This file contains 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
Skipping extended headers under v2.4 presently fails because they are
being handled as if they are identical to v2.3. But they are not:
In v2.3, the extended header size excludes itself.
In v2.4, the extended header size includes itself, and is a synchsafe
integer.
Add test to read v2.4 file with extended header. The test file 'v2.4-ext-header.mp3' has been generated by preparing a short (3 audio frames) mp3 file tagged under ID3 v2.4 with a single 'Blues' genre frame, and an ID3 CRC extended header.
Add tests to read v2.4 files with invalid extended header. The test file 'v2.4-ext-header-invalid-too-short.mp3' is a copy of test file 'v2.4-ext-header.mp3', but with the extended header size edited to become 3. This too short, it must be a minimum of 4 bytes. The test file 'v2.4-ext-header-invalid.mp3' is a copy of test file 'v2.4-ext-header.mp3', but with the extended header size edited to become 1 more than the size of the entire id3 tag. This is too long.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
At present,
Audio::Scanfails to extract tags from a file tagged with ID3 v2.4 when the tag includes an ID3 extended header. An extended header will be encountered when the tag carries a CRC.Audio::Scanhas no interest in the contents of an extended header, but it needs to be able to skip over it to identify and extract the ID3 frames that follow. The skip logic succeeds with a v2.3 tag, but fails with a v2.4 tag, because of changes in the way extended headers are handled between v2.3 and v2.4.This proposed change corrects matters by distinguishing the two variants, and handling them accordingly. The logic is implemented in
_id3_parse_v2, insrc/id3.cThree further test cases have also been added to handle v2.4 extended headers, supplementing the existing extended header tests which are relevant to v2.3.
Details may be found in section 3.2 Extended header of each of the published ID3 documents:
ID3 v2.3: https://id3.org/d3v2.3.0
ID3 v2.4: https://id3.org/id3v2.4.0-structure
The issue was brought to light in a recent Squeezebox forum post: Should LMS read metadata from ID3v2.4?
https://forums.slimdevices.com/showthread.php?115010-Should-LMS-read-metadata-from-ID3v2-4
The thread originator is using the
mp3fsfilesystem to generate/transcode mp3 files on the fly from flac source files. It happens thatmp3fstags the generated mp3 files with ID3 v2.4 tags and includes an ID3 CRC. In consequence,Audio::Scanfailed to extract the tags.I suspect that ID3 CRC's are not otherwise commonly found in the wild.
I have tested the proposed change over a reasonably sized collection of mp3 files tagged & CRC'd under v2.4, and it works as expected. But the collection is not particularly diverse, so surprises may yet emerge. The ID3 documentation does seem to be clear on how the logic should work, and that gives me confidence in the proposed change.
Mentioning @mherger and @ralph-irving as they expressed interest in the forum post.