Add support for ID3v2.4 multi-value tags#10799
Merged
rohitjoins merged 11 commits intogoogle:dev-v2from Nov 29, 2022
Merged
Conversation
Add support for multi-value tags based on null terminators. These are specific to ID3v2.4, but is backwards compatible with ID3v2.3, so no version checks are needed.
Clarify that null-termianted multi-value separators are specific to ID3v2.4.
Rename indexOfEos to indexOfTerminator to better reflect how a terminator != the end of a frame now.
Fix second-order issues stemming from the addition of multi-value tags.
Add tests for ID3v2 multi-value text frames.
icbaker
requested changes
Nov 21, 2022
Collaborator
icbaker
left a comment
icbaker
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I added some initial thoughts
library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java
Outdated
Show resolved
Hide resolved
library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java
Outdated
Show resolved
Hide resolved
...extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java
Outdated
Show resolved
Hide resolved
library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java
Show resolved
Hide resolved
library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java
Outdated
Show resolved
Hide resolved
...extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java
Outdated
Show resolved
Hide resolved
...extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java
Outdated
Show resolved
Hide resolved
...extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java
Outdated
Show resolved
Hide resolved
...extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java
Outdated
Show resolved
Hide resolved
Rework the interface for ID3v2 text frames by: - Switching from a mutable array to ImmutableList - Never leaving the array empty, instead filling it with "" if there are no values This makes the surface safer and more in line with the rest of the module.
Rework the parsing of multi-value text frames to reduce duplication and increase efficiency.
Make the values of TextInformationFrame an abstract list instead of an explicit ImmutableList.
Fix a constructor usage that tried to use an array instead of a list.
icbaker
requested changes
Nov 22, 2022
...extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java
Show resolved
Hide resolved
...extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java
Outdated
Show resolved
Hide resolved
library/extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/Id3Decoder.java
Outdated
Show resolved
Hide resolved
Instead of the ID3v2 text frame falling back to a singleton list of an empty string when the given values are empty, make that case throw an exception within the text frame, and move that fallback behavior into the ID3v2 decoder.
Collaborator
|
Looks good, thanks for all the changes! I'll get this merged. |
icbaker
approved these changes
Nov 23, 2022
icbaker
reviewed
Nov 24, 2022
...extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java
Show resolved
Hide resolved
icbaker
reviewed
Nov 25, 2022
...extractor/src/main/java/com/google/android/exoplayer2/metadata/id3/TextInformationFrame.java
Show resolved
Hide resolved
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 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.
The problem
My app uses ExoPlayer's metadata parser to work around issues with
MediaStore's metadata handling. However, recent developments have required me to start handling multi-value tags, as many users want my app to support multiple artist/genre entries.While the vorbis extractor supports multiple instances of the tag, the ID3v2 extractor does not, even though ID3v2.4 specifies that text information frames can have multiple values in them if delimited by a null terminator. This worsens the user experience with multi-value tags.
What this PR does
This PR deprecates the
valuefield inTextInformationFrameand replaces it with a new array field calledvalues.Id3Decoderpopulates this field with multiple values if it finds multiple tags delimited by null terminators. The now-deprecatedvaluefield is populated with the first item invalues, to retain compatibility with the old implementation that would parse the first value only.This new algorithm should be backwards compatible with ID3v2.3 and ID3v2.2, as they only leverage null terminators as an informal shortcut to make serializing to C strings easier.
Further notes
I've modified other parts of the codebase to use the non-deprecated value to the most backwards-compatible extent possible. I have also added tests into
Id3Decoderto handle the new functionality.