search

alfg / mp4-rust

MP4 reader + writer library in Rust! 🎥🦀
https://crates.io/crates/mp4
MIT License
296 stars 81 forks source link

Allow Hdlr to be not the first in the Meta box #95

Closed DCNick3 closed 1 year ago

DCNick3 commented 1 year ago

The spec seems to allow the hdlr box to be located at any place in the meta box. This commit addresses that.

DCNick3 commented 1 year ago

cc @jessa0 (as you are the one who wrote the code originally in #91): is assuming that contents of the Meta box are all boxes a sane approach? I don't think this can be parsed in any other way..

jessa0 commented 1 year ago

The spec seems to allow the hdlr box to be located at any place in the meta box.

I'm not sure how you're interpreting the spec in that way? According to the syntax description in ISO 14496-12-2012 section 8.11.1.2, the HandlerBox comes first in the MetaBox, followed by a bunch of optional typed boxes, and a Box[] at the end. I don't have access to a spec of the syntax description language, but everywhere else in the spec, ordering in the class determines the order of data in the serialized file..

DCNick3 commented 1 year ago

Oh, I've actually look at the verbal description instead of the syntactic one. The verbal one said "The 'meta' box is required to contain a ‘hdlr’ box indicating the structure or format of the ‘meta’ box contents.", so I figured "contain" is "contains anywhere"

The syntactic one does give an explicit order though...

So... Apparently this is not according to standard, but I do have a file, which seems to be produced by TMPGEnc Video Mastering Works 7 Version 7.0.15.17 (as indicated by the metadata) which does have the ilst box before the HandlerBox

I think it's still a good idea to support it, as the spec still guarantees that the contents of the MetaBox are all boxes

jessa0 commented 1 year ago

So... Apparently this is not according to standard, but I do have a file, which seems to be produced by TMPGEnc Video Mastering Works 7 Version 7.0.15.17 (as indicated by the metadata) which does have the ilst box before the HandlerBox

Oh yikes, so I see we're out of spec territory then. Since you do have a sample from a real encoder with this behavior, I do agree with "being liberal with what you accept" here.

To actually answer your original question, yes, all the data should at least be Boxes, so your PR seems like an improvement in that respect.

DCNick3 commented 1 year ago

Rebased and squashed, CI fails again due to an unrelated lint =(

alfg commented 1 year ago

Thanks @DCNick3 and @jessa0.