Restore read_moov behavior allowing multiple moov boxes #263
Conversation
The behavior change was introduced by c196ca8 Despite being invalid according to ISOBMFF (ISO 14496-12:2015) § 8.2.1.1, this restores the original behavior, while maintaining the interface that eliminates the necessity for callers to create a `MediaContext` object to pass in.
| impl MediaContext { | ||
| pub fn new() -> MediaContext { | ||
| Default::default() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This could've been removed in c196ca8, but better late than never
| @@ -618,6 +618,24 @@ fn public_video_av1() { | |||
| } | |||
| } | |||
|
|
|||
| #[test] | |||
| fn public_mp4_bug_1185230() { | |||
There was a problem hiding this comment.
I wanted to do more here, but it was turning into a yak shave, so I've filed #264
There was a problem hiding this comment.
I'm fine with reverting the behaviour, but it may be worth investigating whether we depend on this functionality for media in the wild.
It's not clear to me if this is a concession to the reality of pervasive invalid MP4 files encoded this way in the wild, or if we just happened to create an invalid test case, but for the sake of not blocking other work, this restores the previous behavior, but with a slightly different approach.
The original file attached to BMO bug 1185230 looks like the result of a fuzzing tool pasting a seed file together twice, so it seems to be an invalid testcase rather than a concession to media in the wild. Splitting the file in half (offset 13651) and comparing the two halves reveals only 6 differing locations in sequences of 2, 8, 4, 4, 1, and 2 bytes - 3 changes inside an mdat binary blob, a key bug triggering change to the time scale of an mdhd (3523726793 vs. 44100), and 2 inside a data element.
test_case_1185230.mp4 seems to be a remuxed derivative of that file with mdatss removed and some other rearrangement, but I'm not sure exactly how it was produced.
| @@ -2278,7 +2272,7 @@ pub fn read_mp4<T: Read>(f: &mut T) -> Result<MediaContext> { | |||
| debug!("{:?}", ftyp); | |||
| } | |||
| BoxType::MovieBox => { | |||
| context = Some(read_moov(&mut b)?); | |||
| context = Some(read_moov(&mut b, context)?); | |||
There was a problem hiding this comment.
I guess the passed context here solves the problem?
|
For the sake of unblocking the AVIF work, I'm merging this for now. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1682376 to consider changing the behavior in the future. |
Resolves #261
https://treeherder.mozilla.org/jobs?repo=try&revision=96f98a79084b97402f73c8d3155d5889bdd75d35 is testing this applied to the mozilla-central tree and looks good. (Note that the
ImageDecoders.AVIFStackCheckerror on Windows is expected)The behavior change was introduced by c196ca8
Despite being invalid according to ISOBMFF (ISO 14496-12:2015) § 8.2.1.1, this restores the original behavior, while maintaining the interface that eliminates the necessity for callers to create a
MediaContextobject to pass in.A brief note about the approach here:
The original code for
read_moovtook a&mut MediaContextparameter and returned aResult<()>. This was fairly natural when the caller (read_mp4) also took a&mut MediaContextparameter. The point of #252 was to improve ergonomics by replacing the in-out context parameters andResult<()>returns with functions that returned the context (inside aResult). There are a number of benefits to this approach. It's less code sinceis reduced to
and it eliminates the possibility that an empty/invalid context even exists to be operated on. However, in the case of
read_moov, despite the fact that ISOBMFF (ISO 14496-12:2015) § 8.2.1.1 states there should be "exactly one" moov box at the file level, at least one C++ test case depends on the parser accepting a file with multiple moov boxes and merging their content. It's not clear to me if this is a concession to the reality of pervasive invalid MP4 files encoded this way in the wild, or if we just happened to create an invalid test case, but for the sake of not blocking other work, this restores the previous behavior, but with a slightly different approach.The previous signature was
and the new one is
Though this has the slightly unconventional approach of taking an owned
Option<MediaContext>and returning aResult<MediaContext>, it has the benefit of not requiring the caller to ever create aMediaContextobject, and since in the case of aSome(_)argument forcontext, that same object is returned, performance should be similar to using a&mut MediaContextparameter.