Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement parsing for iTunes metadata atoms #182

Merged
merged 17 commits into from Jul 16, 2019
Merged

Conversation

chyyran
Copy link
Contributor

@chyyran chyyran commented Jul 9, 2019

Fixes #150.

Metadata in .MP4 files can be stored in either iTunes-style, or 3GPP-style formats. The most commonly found today is the iTunes-style format, which this Pull Request implements parsing for. This is useful for mp4parse to serve as a Rust library that can read these tags off MP4 audio and video files, and can be used as a foundation for a pure-Rust TagLib-like library, along with other metadata-parsing libraries available on Cargo.

iTunes metadata is stored within a Metadata meta atom within a root-level User data udta atom. While this meta atom may contain other atoms such as keys, we are mostly concerned with the ilst List atom, which holds a list of various named atoms that represent metadata.

Each metadata box or entry within ilst has a unique key and is parsed as a full box, within which a data atom that contains the actual data, in either a string or binary u8 format, is contained. Each metadata entry may contain only one data atom, with the exception of the cover art album covr, which may have multiple data atoms that each contain JPEG or PNG data. With the exception of ©lyr and ldes, string metadata tags are limited to 256 characters in length, while binary metadata often have idiosyncratic formats that must be special-cased.

Parsing for all if not most common tags have been implemented, with the exception of the iTunes specific iTMS xxID entries, due to lack of documentation on these tags.

The udta and meta boxes are not exposed in the C API, since Firefox does not have a current need to use these atoms. They are currently only exposed in the Rust API.

This public domain picture of a black hole is embedded within metadata.mp4 and metadata_gnre.mp4 for testing the covr atom. The test files themselves are just copies of minimal.mp4 with a bunch of tags added to them.

A huge thanks to the AtomicParsley project for documentation on the format of individual tag boxes.

@chyyran chyyran marked this pull request as ready for review Jul 14, 2019
/// is parsed.
#[derive(Debug, Default, Clone)]
pub struct UserdataBox {
pub meta: Option<MetadataBox>
Copy link
Contributor Author

@chyyran chyyran Jul 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While calling the udta box userdata in the MediaContext is probably a reasonable decision, I'm not sure if the field for the nested meta box should be named metadata, since "metadata" can be ambiguous between track metadata, and metadata tags relating to the media the file or stream contains. Since the name of the box itself is meta, and it's a relatively self-explanatory FourCC, keeping it as meta makes sense, but naming it metadata would also be more consistent.

If you have an opinion either way, would be happy to change it.

@chyyran
Copy link
Contributor Author

chyyran commented Jul 14, 2019

@kinetiknz could I get a review please?

Copy link
Collaborator

@kinetiknz kinetiknz left a comment

Thanks for the PR! This generally looks good; a few nits and one important issue/query to resolve.

Ok(())
}

fn read_bool_data<T: Read>(src: &mut BMFFBox<T>) -> Result<Option<bool>> {
Copy link
Collaborator

@kinetiknz kinetiknz Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename these new read_* additions to read_ilst_* or move them inside read_ilst to make it clear they're ILST-specific.

fn read_data<T: Read>(src: &mut BMFFBox<T>) -> Result<Vec<u8>> {
// Skip past the padding bytes
skip(&mut src.content, src.head.offset as usize)?;
let size = src.content.limit() as usize;
Copy link
Collaborator

@kinetiknz kinetiknz Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra space after =.

)
}


Copy link
Collaborator

@kinetiknz kinetiknz Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop double newlines here and below.

}


fn read_multiple_u8_data<T: Read>(src: &mut BMFFBox<T>) -> Result<Vec<Vec<u8>>> {
Copy link
Collaborator

@kinetiknz kinetiknz Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice if we could make most of read_multiple_u8_data and read_u8_data common.

@@ -773,6 +947,11 @@ fn read_moov<T: Read>(f: &mut BMFFBox<T>, context: &mut MediaContext) -> Result<
debug!("{:?}", pssh);
vec_push(&mut context.psshs, pssh)?;
}
BoxType::UserdataBox => {
let udta = read_udta(&mut b)?;
Copy link
Collaborator

@kinetiknz kinetiknz Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My primary concern with these additions is that the main user of mp4parse-rust (Gecko) is exposed to new forms of parse failure if invalid udta (or children) are encountered. We don't want to reject MP4s we previously accepted due to content Gecko isn't interested in.

I think a simple solution to this is to make MediaContext::userdata a Result<Option<Userdata>> and avoid bubbling an error result up from here instead. That does make the MediaContext API slightly odd, though.

Copy link
Contributor Author

@chyyran chyyran Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of Result<Option<UserdataBox>>, I elected to make it Option<Result<UserdataBox>> instead, since Result<Option<_>> doesn't implement Default.

I considered just ignoring the error and returning None if udta parsing fails, so as to not introduce any weirdness into the MediaContext API, but that seems to me that it would conflate the lack of an udta atom with a corrupt udta atom.

chyyran added 2 commits Jul 15, 2019
@chyyran chyyran force-pushed the box_meta branch 2 times, most recently from 47f8d95 to 275b1e0 Compare Jul 15, 2019
@kinetiknz
Copy link
Collaborator

kinetiknz commented Jul 16, 2019

Thanks, looks good!

@kinetiknz kinetiknz merged commit 201556d into mozilla:master Jul 16, 2019
1 check passed
kinetiknz added a commit to kinetiknz/mp4parse-rust that referenced this issue Jan 17, 2020
With PR mozilla#182, mp4parse will now attempt to read metadata, including
potential large items like cover art.  With read_buf's previous limit of
1MB, this would be fairly trivial to hit with a large cover art image.  This
addresses a parsing failure reported in BMO 1609573.  In the event that 10MB
is still too restrictive, it probably makes sense to audit and split
read_buf uses into small and large size classes.
kinetiknz added a commit that referenced this issue Jan 17, 2020
With PR #182, mp4parse will now attempt to read metadata, including
potential large items like cover art.  With read_buf's previous limit of
1MB, this would be fairly trivial to hit with a large cover art image.  This
addresses a parsing failure reported in BMO 1609573.  In the event that 10MB
is still too restrictive, it probably makes sense to audit and split
read_buf uses into small and large size classes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants