From 6ea39851a54c3c61ec41d69de53e9ba5cdce9e40 Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Thu, 29 Oct 2026 16:28:02 -0600 Subject: [PATCH 1/2] S3 Perf (38.3.1.16): Record the metadata of a video carrying a track the MP4 reader does not recognize An MP4 or QuickTime video with a track the MP4 reader cannot name, such as a timecode track, got no video metadata even when it had a video track, since that one track failed the whole file. Such a video now gets its video track's duration, width, and height, on `oxen add` and on workspace uploads alike, self-hosted and on Oxen.ai. A video with several video tracks gets the metadata of the one with the lowest track ID, where it took whichever the reader listed first, which could differ from one run to the next. A video whose movie header has a timescale of 0 now gets no metadata instead of panicking. ENG-2318 Sentry: OXEN-SERVER-4M --- .../src/repositories/metadata/video.rs | 64 +++++++++++++----- data/test/video/two_videos_timecode.mp4 | Bin 0 -> 4589 bytes 2 files changed, 46 insertions(+), 18 deletions(-) create mode 100644 data/test/video/two_videos_timecode.mp4 diff --git a/crates/liboxen/src/repositories/metadata/video.rs b/crates/liboxen/src/repositories/metadata/video.rs index 5773faa2e7..8532cf80c4 100644 --- a/crates/liboxen/src/repositories/metadata/video.rs +++ b/crates/liboxen/src/repositories/metadata/video.rs @@ -3,8 +3,9 @@ use crate::{error::OxenError, model::metadata::MetadataVideo}; -use mp4::{Mp4Reader, Mp4Track, TrackType}; +use mp4::{Mp4Reader, TrackType}; use std::io::{BufReader, Read, Seek, SeekFrom}; +use std::time::Duration; /// Detects the video metadata of the file `data` reads. pub fn get_metadata(mut data: impl Read + Seek) -> Result { @@ -13,28 +14,26 @@ pub fn get_metadata(mut data: impl Read + Seek) -> Result { - let duration = video.duration().as_secs_f64(); - - let video_tracks: Vec<&Mp4Track> = video + // `Mp4Reader::duration` panics on a timescale of 0 + let mvhd = &video.moov.mvhd; + let millis = (mvhd.duration.checked_mul(1000)) + .and_then(|duration| duration.checked_div(u64::from(mvhd.timescale))) + .ok_or_else(|| OxenError::internal_error("Could not get video duration"))?; + let duration = Duration::from_millis(millis).as_secs_f64(); + + // The video track with the lowest ID, passing over tracks whose type the reader cannot + // name, such as timecode tracks + let track = video .tracks() .values() - .filter_map(|t| match t.track_type() { - Ok(TrackType::Video) => Some(Ok(t)), - Ok(_) => None, - Err(e) => Some(Err(OxenError::basic_str(format!( - "Could not get track type: {e:?}" - )))), - }) - .collect::, _>>()?; - - let video = video_tracks - .first() - .ok_or_else(|| OxenError::basic_str("Could not get video track"))?; + .filter(|track| matches!(track.track_type(), Ok(TrackType::Video))) + .min_by_key(|track| track.track_id()) + .ok_or_else(|| OxenError::internal_error("Could not get video track"))?; Ok(MetadataVideo::new( duration, - video.width() as usize, - video.height() as usize, + track.width() as usize, + track.height() as usize, )) } Err(err) => { @@ -53,6 +52,7 @@ mod tests { use crate::test; use approx::assert_relative_eq; + use std::io::Cursor; #[test] fn test_get_metadata_video_mp4() { @@ -72,6 +72,34 @@ mod tests { assert_eq!(metadata.video.width, 128); assert_eq!(metadata.video.height, 176); assert_relative_eq!(metadata.video.num_seconds, 1.6); + + // A video with timecode tracks, whose type the MP4 reader cannot name, gets the metadata + // of its first video track: track 1 is 32x24 and track 2 is 16x16. + let file = test::test_video_file_with_name("two_videos_timecode.mp4"); + let metadata = repositories::metadata::get(file).unwrap(); + let Some(GenericMetadata::MetadataVideo(metadata)) = metadata.metadata else { + panic!( + "two_videos_timecode.mp4 has video metadata, not {:?}", + metadata.metadata + ); + }; + assert_eq!((metadata.video.width, metadata.video.height), (32, 24)); + assert_relative_eq!(metadata.video.num_seconds, 1.0); + + // A video whose movie header has a timescale of 0 fails to give metadata rather than + // panicking. In a version 0 header the timescale follows the version and flags and two + // 4-byte timestamps. + let mut bytes = std::fs::read(test::test_video_file_with_name("basketball.mp4")).unwrap(); + let mvhd = (bytes.windows(4)) + .position(|fourcc| fourcc == b"mvhd") + .expect("basketball.mp4 has a movie header"); + assert_eq!( + bytes[mvhd + 4], + 0, + "basketball.mp4's movie header is version 0" + ); + bytes[mvhd + 16..mvhd + 20].fill(0); + assert!(super::get_metadata(Cursor::new(bytes)).is_err()); } #[test] diff --git a/data/test/video/two_videos_timecode.mp4 b/data/test/video/two_videos_timecode.mp4 new file mode 100644 index 0000000000000000000000000000000000000000..1273b7a90f415be9297cc6cc7062495ed5b092df GIT binary patch literal 4589 zcmeHKeQ;FO6~CJVS<+w`Cv-Cp0fcO+g&Lb1bO^!gWqd!e?fhVrn(=J!wqxb5Xw> zHlBptd{uF5Q1jHN?IbvfNNHX7c+3TkrG_EPadesXjyN*>x}r&-xvoskM+w*^Y|Rl& zjFm=|sFk4bXx9{VoTm8}yc-QtOo^(b<9bWiV&tfxT~92UzLoh(QZ`RX21E8F&uO!w zJW0ELY&)SS=of6q9zP_~d4>)WGmZ2$lN^pzG=L<~aOK@`MQ!*+3Cc0DKv!wjJ5POape)j$=!=1K$Il1pWk^V=Nz;%ZLB` z?ZDxSRC7jhiNrHSUt%@qVGROfmr~70?Eh0W=OGnT$pjkzO*P;4s~f16k)cf1w6tM+ ze(!%$&4p`|5qu%ld^LPu2~b_+1G50CVCs4_6R6TBRrB;ln4r(+Jp>E@BQWQ{YT#~Q z4={wR?wiz`)2lhvr!TRZ5mah?Mm0l&?lNxTOx7iYh};XP8}4HSBP|Ae5K-1%D-& z5yvrW%E~%AI=u9`V??Ex*RYz)$cs1P=uzkxrlT1#yN1hBNRmY#$0v>GtKy0piW+i@ z=&KQG1TMv-XqT;uLT7o6(CI5L=ejDk=@D+Xg+TcPZrWWi#xrOsqR%V93JRm(O<#kh-K(fkZd_V>7r517X7@svs#fIu;f-<6cB@?bQ>`k_^$W)cv!NX zplP>gCTS!WXf=aIIBctqSnB6a#DawlY1A-UqzGPu6FfVr$%ti~A@G=$@F8ou;r`4o|6SLGQR(%Z& z+fhxif@>xY2_=CugfFQT1wg0CdQsW?)Qub1%=qiZ^6u{9+)s*UG?yNJ={K9|KN#vh zbNso%S?lgzwq@tpXPh6LUVGi#1&=T3+xv~TU;E+0v8!MH?IVw`-F3Uar^-JzJQmtE z`;QF=>w?zS!X{;7epBeZr}hqS{z_%*re7Y|(k67&%-XoS_EZ2v zDB9`YQTuGq27FFkzGs*3+5g%-Q|X?~jStOi8$H#W`J**|9@}>7N$>kdgzU`kK6J&= zCw6NuyT0C6^=8Z8pM9@q|Hl2hezN-H!-v0h?Y2A0IyNny^6dq=s|W9DsvTYy{73gt z2J3&7`YOJFF>hc5q zd8_}%l~<+32UZR5ddc1jX3hljIPMnn1o}PNQg`8vPh`DwFl*`gz}-?xOn_^)|+LllhDZ&J8^$7L3M^?gqCo&|mW8L3h#nuddIk|F!$h zr$1^hzHb9x`_rCHkG?qe3N-T4HBRss{|esBK)>+HS$EOL{s-BLo`EF=jf3=malr-b zGwW|Ub%#Xia|8X=Bll)ZjUyV@Rq=t>3zqLW>3Qz9RkP>3kn}Jio)iBIMGLP1_rowj sU@ZTg1HGT-ogm(x~;r=droxZFL1W21Io?Pf@j(=^Gno)3Hf1@Y$`wEzGB literal 0 HcmV?d00001 From 0fb606d6a6e522ae0544a66ab56fe6151164e072 Mon Sep 17 00:00:00 2001 From: Nathan Stocks Date: Thu, 29 Oct 2026 17:04:10 -0600 Subject: [PATCH 2/2] Use expect in the video metadata test, and say the reader does not recognize a timecode track --- crates/liboxen/src/repositories/metadata/video.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/crates/liboxen/src/repositories/metadata/video.rs b/crates/liboxen/src/repositories/metadata/video.rs index 8532cf80c4..dae2e956e4 100644 --- a/crates/liboxen/src/repositories/metadata/video.rs +++ b/crates/liboxen/src/repositories/metadata/video.rs @@ -21,8 +21,8 @@ pub fn get_metadata(mut data: impl Read + Seek) -> Result