Make parsers return context objects rather that using out params

This simplifies the caller by eliminating the need to create the context,
ensures that an empty/invalid context doesn't exist to accidentally operate on,
and makes the code more idiomatic rust.

Also, make some other minor cleanups.

Author: baumanj

mp4parse/benches/avif_benchmark.rs

@@ -15,10 +15,9 @@ criterion_group!(benches, criterion_benchmark);
 criterion_main!(benches);
 
 fn avif_largest() {
-    let context = &mut mp4::AvifContext::new();
     let input = &mut File::open(
         "av1-avif/testFiles/Netflix/avif/cosmos_frame05000_yuv444_12bpc_bt2020_pq_qlossless.avif",
     )
     .expect("Unknown file");
-    assert!(mp4::read_avif(input, context).is_ok());
+    assert!(mp4::read_avif(input).is_ok());
 }

mp4parse/src/lib.rs

@@ -1218,10 +1218,7 @@ fn skip_box_remain<T: Read>(src: &mut BMFFBox<T>) -> Result<()> {
 }
 
 /// Read the contents of an AVIF file
-///
-/// Metadata is accumulated in the passed-through `AvifContext` struct,
-/// which can be examined later.
-pub fn read_avif<T: Read>(f: &mut T, context: &mut AvifContext) -> Result<()> {
+pub fn read_avif<T: Read>(f: &mut T) -> Result<AvifContext> {
     let _ = env_logger::try_init();
 
     let mut f = OffsetReader::new(f);
@@ -1297,20 +1294,23 @@ pub fn read_avif<T: Read>(f: &mut T, context: &mut AvifContext) -> Result<()> {
         return Err(Error::InvalidData("multiple alpha planes"));
     }
 
-    context.premultiplied_alpha = alpha_item_id.map_or(false, |alpha_item_id| {
+    let premultiplied_alpha = alpha_item_id.map_or(false, |alpha_item_id| {
         meta.item_references.iter().any(|iref| {
             iref.from_item_id == meta.primary_item_id
                 && iref.to_item_id == alpha_item_id
                 && iref.item_type == b"prem"
         })
     });
 
+    let mut primary_item = TryVec::new();
+    let mut alpha_item = None;
+
     // load data of relevant items
     for loc in meta.iloc_items.iter() {
         let mut item_data = if loc.item_id == meta.primary_item_id {
-            &mut context.primary_item
+            &mut primary_item
         } else if Some(loc.item_id) == alpha_item_id {
-            context.alpha_item.get_or_insert_with(TryVec::new)
+            alpha_item.get_or_insert_with(TryVec::new)
         } else {
             continue;
         };
@@ -1340,7 +1340,11 @@ pub fn read_avif<T: Read>(f: &mut T, context: &mut AvifContext) -> Result<()> {
         }
     }
 
-    Ok(())
+    Ok(AvifContext {
+        primary_item,
+        alpha_item,
+        premultiplied_alpha,
+    })
 }
 
 /// Parse a metadata box in the context of an AVIF
@@ -2040,12 +2044,9 @@ fn read_iloc<T: Read>(src: &mut BMFFBox<T>) -> Result<TryVec<ItemLocationBoxItem
 }
 
 /// Read the contents of a box, including sub boxes.
-///
-/// Metadata is accumulated in the passed-through `MediaContext` struct,
-/// which can be examined later.
-pub fn read_mp4<T: Read>(f: &mut T, context: &mut MediaContext) -> Result<()> {
+pub fn read_mp4<T: Read>(f: &mut T) -> Result<MediaContext> {
+    let mut context = None;
     let mut found_ftyp = false;
-    let mut found_moov = false;
     // TODO(kinetik): Top-level parsing should handle zero-sized boxes
     // rather than throwing an error.
     let mut iter = BoxIter::new(f);
@@ -2072,13 +2073,12 @@ pub fn read_mp4<T: Read>(f: &mut T, context: &mut MediaContext) -> Result<()> {
                 debug!("{:?}", ftyp);
             }
             BoxType::MovieBox => {
-                read_moov(&mut b, context)?;
-                found_moov = true;
+                context = Some(read_moov(&mut b)?);
             }
             _ => skip_box_content(&mut b)?,
         };
         check_parser_state!(b.content);
-        if found_moov {
+        if context.is_some() {
             debug!(
                 "found moov {}, could stop pure 'moov' parser now",
                 if found_ftyp {
@@ -2093,56 +2093,64 @@ pub fn read_mp4<T: Read>(f: &mut T, context: &mut MediaContext) -> Result<()> {
     // XXX(kinetik): This isn't perfect, as a "moov" with no contents is
     // treated as okay but we haven't found anything useful.  Needs more
     // thought for clearer behaviour here.
-    if found_moov {
-        Ok(())
-    } else {
-        Err(Error::NoMoov)
-    }
+    context.ok_or(Error::NoMoov)
 }
 
-fn parse_mvhd<T: Read>(f: &mut BMFFBox<T>) -> Result<(MovieHeaderBox, Option<MediaTimeScale>)> {
+fn parse_mvhd<T: Read>(f: &mut BMFFBox<T>) -> Result<Option<MediaTimeScale>> {
     let mvhd = read_mvhd(f)?;
+    debug!("{:?}", mvhd);
     if mvhd.timescale == 0 {
         return Err(Error::InvalidData("zero timescale in mdhd"));
     }
     let timescale = Some(MediaTimeScale(u64::from(mvhd.timescale)));
-    Ok((mvhd, timescale))
+    Ok(timescale)
 }
 
-fn read_moov<T: Read>(f: &mut BMFFBox<T>, context: &mut MediaContext) -> Result<()> {
+fn read_moov<T: Read>(f: &mut BMFFBox<T>) -> Result<MediaContext> {
+    let MediaContext {
+        mut timescale,
+        mut tracks,
+        mut mvex,
+        mut psshs,
+        mut userdata,
+    } = Default::default();
+
     let mut iter = f.box_iter();
     while let Some(mut b) = iter.next_box()? {
         match b.head.name {
             BoxType::MovieHeaderBox => {
-                let (mvhd, timescale) = parse_mvhd(&mut b)?;
-                context.timescale = timescale;
-                debug!("{:?}", mvhd);
+                timescale = parse_mvhd(&mut b)?;
             }
             BoxType::TrackBox => {
-                let mut track = Track::new(context.tracks.len());
+                let mut track = Track::new(tracks.len());
                 read_trak(&mut b, &mut track)?;
-                context.tracks.push(track)?;
+                tracks.push(track)?;
             }
             BoxType::MovieExtendsBox => {
-                let mvex = read_mvex(&mut b)?;
+                mvex = Some(read_mvex(&mut b)?);
                 debug!("{:?}", mvex);
-                context.mvex = Some(mvex);
             }
             BoxType::ProtectionSystemSpecificHeaderBox => {
                 let pssh = read_pssh(&mut b)?;
                 debug!("{:?}", pssh);
-                context.psshs.push(pssh)?;
+                psshs.push(pssh)?;
             }
             BoxType::UserdataBox => {
-                let udta = read_udta(&mut b);
-                debug!("{:?}", udta);
-                context.userdata = Some(udta);
+                userdata = Some(read_udta(&mut b));
+                debug!("{:?}", userdata);
             }
             _ => skip_box_content(&mut b)?,
         };
         check_parser_state!(b.content);
     }
-    Ok(())
+
+    Ok(MediaContext {
+        timescale,
+        tracks,
+        mvex,
+        psshs,
+        userdata,
+    })
 }
 
 fn read_pssh<T: Read>(src: &mut BMFFBox<T>) -> Result<ProtectionSystemSpecificHeaderBox> {

mp4parse/src/tests.rs

@@ -7,7 +7,6 @@
 
 use super::read_mp4;
 use super::Error;
-use super::MediaContext;
 use fallible_collections::TryRead as _;
 
 use std::convert::TryInto as _;
@@ -195,8 +194,7 @@ fn read_truncated_ftyp() {
             .B32(0) // minor version
             .append_bytes(b"isom")
     });
-    let mut context = MediaContext::new();
-    match read_mp4(&mut stream, &mut context) {
+    match read_mp4(&mut stream) {
         Err(Error::UnexpectedEOF) => (),
         Ok(_) => panic!("expected an error result"),
         _ => panic!("expected a different error result"),
@@ -346,7 +344,7 @@ fn read_mdhd_invalid_timescale() {
     assert_eq!(stream.head.name, BoxType::MediaHeaderBox);
     assert_eq!(stream.head.size, 44);
     let r = super::parse_mdhd(&mut stream, &mut super::Track::new(0));
-    assert_eq!(r.is_err(), true);
+    assert!(r.is_err());
 }
 
 #[test]
@@ -387,7 +385,7 @@ fn read_mvhd_invalid_timescale() {
     assert_eq!(stream.head.name, BoxType::MovieHeaderBox);
     assert_eq!(stream.head.size, 120);
     let r = super::parse_mvhd(&mut stream);
-    assert_eq!(r.is_err(), true);
+    assert!(r.is_err());
 }
 
 #[test]
@@ -1249,9 +1247,8 @@ fn read_invalid_pssh() {
     let mut stream = make_box(BoxSize::Auto, b"moov", |s| s.append_bytes(pssh.as_slice()));
     let mut iter = super::BoxIter::new(&mut stream);
     let mut stream = iter.next_box().unwrap().unwrap();
-    let mut context = super::MediaContext::new();
 
-    match super::read_moov(&mut stream, &mut context) {
+    match super::read_moov(&mut stream) {
         Err(Error::InvalidData(s)) => assert_eq!(s, "read_buf size exceeds BUF_SIZE_LIMIT"),
         _ => panic!("unexpected result with invalid descriptor"),
     }

mp4parse/tests/public.rs

@@ -43,8 +43,7 @@ fn public_api() {
     fd.read_to_end(&mut buf).expect("File error");
 
     let mut c = Cursor::new(&buf);
-    let mut context = mp4::MediaContext::new();
-    mp4::read_mp4(&mut c, &mut context).expect("read_mp4 failed");
+    let context = mp4::read_mp4(&mut c).expect("read_mp4 failed");
     assert_eq!(context.timescale, Some(mp4::MediaTimeScale(1000)));
     for track in context.tracks {
         match track.track_type {
@@ -164,8 +163,7 @@ fn public_metadata() {
     fd.read_to_end(&mut buf).expect("File error");
 
     let mut c = Cursor::new(&buf);
-    let mut context = mp4::MediaContext::new();
-    mp4::read_mp4(&mut c, &mut context).expect("read_mp4 failed");
+    let context = mp4::read_mp4(&mut c).expect("read_mp4 failed");
     let udta = context
         .userdata
         .expect("didn't find udta")
@@ -232,8 +230,7 @@ fn public_metadata_gnre() {
     fd.read_to_end(&mut buf).expect("File error");
 
     let mut c = Cursor::new(&buf);
-    let mut context = mp4::MediaContext::new();
-    mp4::read_mp4(&mut c, &mut context).expect("read_mp4 failed");
+    let context = mp4::read_mp4(&mut c).expect("read_mp4 failed");
     let udta = context
         .userdata
         .expect("didn't find udta")
@@ -301,8 +298,7 @@ fn public_audio_tenc() {
     fd.read_to_end(&mut buf).expect("File error");
 
     let mut c = Cursor::new(&buf);
-    let mut context = mp4::MediaContext::new();
-    mp4::read_mp4(&mut c, &mut context).expect("read_mp4 failed");
+    let context = mp4::read_mp4(&mut c).expect("read_mp4 failed");
     for track in context.tracks {
         let stsd = track.stsd.expect("expected an stsd");
         let a = match stsd.descriptions.first().expect("expected a SampleEntry") {
@@ -360,8 +356,7 @@ fn public_video_cenc() {
     fd.read_to_end(&mut buf).expect("File error");
 
     let mut c = Cursor::new(&buf);
-    let mut context = mp4::MediaContext::new();
-    mp4::read_mp4(&mut c, &mut context).expect("read_mp4 failed");
+    let context = mp4::read_mp4(&mut c).expect("read_mp4 failed");
     for track in context.tracks {
         let stsd = track.stsd.expect("expected an stsd");
         let v = match stsd.descriptions.first().expect("expected a SampleEntry") {
@@ -433,8 +428,7 @@ fn public_audio_cbcs() {
     fd.read_to_end(&mut buf).expect("File error");
 
     let mut c = Cursor::new(&buf);
-    let mut context = mp4::MediaContext::new();
-    mp4::read_mp4(&mut c, &mut context).expect("read_mp4 failed");
+    let context = mp4::read_mp4(&mut c).expect("read_mp4 failed");
     for track in context.tracks {
         let stsd = track.stsd.expect("expected an stsd");
         assert_eq!(stsd.descriptions.len(), 2);
@@ -516,8 +510,7 @@ fn public_video_cbcs() {
     fd.read_to_end(&mut buf).expect("File error");
 
     let mut c = Cursor::new(&buf);
-    let mut context = mp4::MediaContext::new();
-    mp4::read_mp4(&mut c, &mut context).expect("read_mp4 failed");
+    let context = mp4::read_mp4(&mut c).expect("read_mp4 failed");
     for track in context.tracks {
         let stsd = track.stsd.expect("expected an stsd");
         assert_eq!(stsd.descriptions.len(), 2);
@@ -576,8 +569,7 @@ fn public_video_av1() {
     fd.read_to_end(&mut buf).expect("File error");
 
     let mut c = Cursor::new(&buf);
-    let mut context = mp4::MediaContext::new();
-    mp4::read_mp4(&mut c, &mut context).expect("read_mp4 failed");
+    let context = mp4::read_mp4(&mut c).expect("read_mp4 failed");
     for track in context.tracks {
         // track part
         assert_eq!(track.duration, Some(mp4::TrackScaledTime(512, 0)));
@@ -623,41 +615,36 @@ fn public_video_av1() {
 
 #[test]
 fn public_avif_primary_item() {
-    let context = &mut mp4::AvifContext::new();
     let input = &mut File::open(IMAGE_AVIF).expect("Unknown file");
-    mp4::read_avif(input, context).expect("read_avif failed");
+    let context = mp4::read_avif(input).expect("read_avif failed");
     assert_eq!(context.primary_item.len(), 6979);
     assert_eq!(context.primary_item[0..4], [0x12, 0x00, 0x0a, 0x0a]);
 }
 
 #[test]
 fn public_avif_primary_item_split_extents() {
-    let context = &mut mp4::AvifContext::new();
     let input = &mut File::open(IMAGE_AVIF_EXTENTS).expect("Unknown file");
-    mp4::read_avif(input, context).expect("read_avif failed");
+    let context = mp4::read_avif(input).expect("read_avif failed");
     assert_eq!(context.primary_item.len(), 4387);
 }
 
 #[test]
 fn public_avif_bug_1655846() {
-    let context = &mut mp4::AvifContext::new();
     let input = &mut File::open(IMAGE_AVIF_CORRUPT).expect("Unknown file");
-    assert!(mp4::read_avif(input, context).is_err());
+    assert!(mp4::read_avif(input).is_err());
 }
 
 #[test]
 fn public_avif_bug_1661347() {
-    let context = &mut mp4::AvifContext::new();
     let input = &mut File::open(IMAGE_AVIF_CORRUPT_2).expect("Unknown file");
-    assert!(mp4::read_avif(input, context).is_err());
+    assert!(mp4::read_avif(input).is_err());
 }
 
 #[test]
 #[ignore] // Remove when we add support; see https://github.com/mozilla/mp4parse-rust/issues/198
 fn public_avif_primary_item_is_grid() {
-    let context = &mut mp4::AvifContext::new();
     let input = &mut File::open(IMAGE_AVIF_GRID).expect("Unknown file");
-    mp4::read_avif(input, context).expect("read_avif failed");
+    mp4::read_avif(input).expect("read_avif failed");
     // Add some additional checks
 }
 
@@ -675,8 +662,7 @@ fn public_avif_read_samples() {
             continue; // Remove when public_avif_primary_item_is_grid passes
         }
         println!("parsing {:?}", path);
-        let context = &mut mp4::AvifContext::new();
         let input = &mut File::open(path).expect("Unknow file");
-        mp4::read_avif(input, context).expect("read_avif failed");
+        mp4::read_avif(input).expect("read_avif failed");
     }
 }