Repository navigation
Add full vorbis comment support - #9864
Merged
Merged
Conversation
Change how OggReader handles the CommentsHeader instance, enabling VorbisComment tags to be parsed from it.
Move all picture and vorbis frame parsing into a new xiph module. This commit also adds cover frame parsing from vorbis comments as well.
The vorbis tests did not expect metadata normally. Now they do.
Add metadata parsing for the Opus format.
Refactor the overall module to place the unified vorbis tags into a single package called `vorbis`. Also re-intoduce the vorbis tags in their original `flac` module, but deprecate them.
Slightly cleanup VorbisUtil.
Line up this branch with the dev branch instead of the release branch.
Contributor
Author
|
By the way: I will be signing the CLA, give me a second. Edit: I signed the CLA. |
icbaker
requested changes
Jan 17, 2022
Collaborator
|
Thanks for adding this! I took a look through and added some comments - let me know if it seems like i've suggested anything nonsensical :) |
Contributor
Author
|
Thanks for the feedback. I'll get to changing those. |
In the old `flac` module, superclass the deprecated types under the moved types in the `vorbis` module. This ensures backwards compat with existing library users.
Try to eliminate some nitpicks regarding VorbisUtil.
Studio accidentally renamed some of the javadoc to reflect the new vorbis module. Revert it to the dev javadoc, as the ExoPlayer devs don't update the javadoc until a release.
67e1261 messed up some of the other docs. Fix that.
Simplify how the comment header is parsed and eliminate a few possible bugs in the process, such as: - Metadata being overwritten directly by the comments header. - The packet being rewound to 0 if it cannot find a comment header, which might result in the cursor being moved to a bad position.
Group up some other minor changes with the vorbis comment utils.
Collapse the two variations of `VorbisUtil.buildMetadata` into a single method called `VorbisUtil.parseVorbisComments` that only takes a list of vorbis strings, compared to previously where it would take strings and picture frame instance. Any code that relied on the old signature now either concatenates picture frames and vorbis comments or copies vorbis comments into an existing metadata instance.
Comment concatenation was accidentally not added previously, which results in a testing failure.
Fix some more nitpicks I encountered.
icbaker
requested changes
Jan 18, 2022
Instead of having types in flac inherit types in vorbis, make types in vorbis inherit types in flac. This is a bit of a hack and somewhat messy, but it retains backwards compatibility.
Replace the flac module usage in the FLAC extension with the vorbis module.
Collaborator
|
Thanks for all the changes - I'll work on getting this merged. |
icbaker
approved these changes
Jan 18, 2022
Contributor
Author
|
Any updates on when this will be merged? |
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.
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.
This PR fully fleshes out the vorbis comment capabilities with the ExoPlayer metadata retrieval system.
The problem
My music player app needs to extract audio metadata at multiple points, such as with high-quality cover art or ReplayGain tags. However, I began to notice that I could only extract metadata from ID3v2 and FLAC tags, which seemed strange as OGG Vorbis tags are nearly identical to FLAC tags. Looking into this, I discovered that:
OggReaderwould parse vorbis comments and then never expose them.OpusReaderwould just skip vorbis comments entirely.As a result, I decided to implement parsing for both of these data structures.
What this does
This PR extends
VorbisCommentandPictureFrameto OGG Vorbis and OPUS streams, enabling full metadata functionality for those formats.VorbisCommentandPictureFrameinstances have been moved to generic implementations inVorbisUtil.METADATA_BLOCK_PICTURE, then the tag is actually decoded from Base64 and then parsed as if it is aPictureFrame. This is outlined by the specification here.PictureFrameparsing has been split into two blocks.FlacReaderwill first copy the contents of the picture frame into aParseableByteArrayon it's own, and then pass it to the utility afterwards. This is required so that vorbis comments can be transformed into picture frames as well.VorbisReader, onceVorbisSetupis returned, theCommentsHeaderinstance is transformed into aMetadatainstance using the aforementioned utilities. This instance is then provided toSetupData.format.OpusReader, after the identification header frame is parsed, the subsequent frame is checked to be a Vorbis Comment frame. If that is the case, it is parsed with the aforementioned utilities and immediately transformed into aMetadatainstance that is then added to toSetupData.format.metadata.flacmodule have been moved to a new module calledmetadata.vorbis, with the previous module being deprecated. I can change this back if desired.I updated the testing configuration to reflect these changes and confirmed that the test suite succeeds. I'm not fully sure what knock-on effects this might have, but hopefully this change only affects local files, like I intended to. If it's not, then I'm okay with this not being merged.
As for documentation, I don't fully know how to work with javadoc or the jekyll docs, so there's some directories that still correspond to the old
flacmodule name. If you need me to update anything I will.Feel free to raise any concerns with my PR.