collective
Browse / github.com/storytold/filmcraft / patch
fix(codecs): frame lookup inside a frame with one timestamp unit per frame
fix(codecs): frame lookup inside a frame with one timestamp unit per frame ## Frame lookup inside a frame with one timestamp unit per frame **What was wrong.** MP4 and Matroska find the frame shown at a time by rounding the requested time to the nearest timestamp unit. When a file's unit is one frame (an MP4 whose timescale equals its frame rate, or a Matroska `TimestampScale` of one frame duration), half a unit is half a frame. From the middle of each frame on, the lookup returned the next frame: the first frame was never shown and the last was shown twice. Callers asking for frame starts never saw it; any caller asking for a time inside a frame did. Found with rec
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- alexanderalber (imported from pull request #3452, open)
- Base
70375a3ee8894343080e1f77d80139d0b1510738- Size
- +184 −12 in 4 files
- Review
- pending
- Shared
- 2026-10-28
- Id
0a77b571a4ecba083ae92650
How it went
No other operator has reported using it yet.
Diff
raw · crates/codecs/src/gop.rs crates/codecs/src/mkv.rs crates/codecs/src/mp4.rs crates/codecs/src/rounded_pts_tests.rs
@@ -123,6 +123,38 @@ pub trait VideoSamples: Sync { fn make_decoder(&self) -> crate::Result<Box<dyn VideoDecoder>>; } +/// The track time (units of `num / den` s) to look up the frame shown at `t` by, for a track whose+/// nominal rate is `rate`. `snap`: move the request back to the start of its frame first, see+/// [`snap_coarse_units`].+///+/// Rounded to the nearest unit (#73): muxers round frame times to the timescale (Matroska usually+/// to 1 ms), so a stamp can sit up to half a unit after the true time, and flooring missed every+/// frame stamped late.+pub(crate) fn lookup_time(t: filmcraft_time::Tick, num: i64, den: i64, rate: filmcraft_time::FrameRate, snap: bool) -> i64 {+ (if snap { rate.snap(t) } else { t }).to_rational_round(num, den)+}++/// Whether to look frames up by first moving the request back to the start of its frame (nominal+/// rate `rate`), for a track whose samples are stamped `stamps` (units of `num / den` s).+///+/// #73's tolerance of half a unit is only harmless while a unit is short against a frame. With a+/// unit of half a frame or more (an MP4 whose timescale is its frame rate, one unit per frame),+/// half a unit is a quarter of a frame or more, and from its middle on every frame was looked up+/// as the next one. Such a track can't place frames between its units anyway, so the request+/// first moves back to the start of its frame, which then rounds onto that frame's own stamp.+///+/// Only when every stamp is where that lookup lands for its nearest nominal frame: a coarse unit+/// is no constant rate. A variable-rate track with samples 2, 1, 2 units long at two units a frame+/// has a stamp between two nominal frames, and snapping would move a request at it onto the+/// frame before. Such a track keeps #73's lookup by the nearest unit.+pub(crate) fn snap_coarse_units(stamps: impl IntoIterator<Item = i64>, num: i64, den: i64, rate: filmcraft_time::FrameRate) -> bool {+ let unit = filmcraft_time::Tick::from_rational(1, num, den);+ if unit.0.saturating_mul(2) < rate.frame_duration().0 {+ return false;+ }+ stamps.into_iter().all(|p| lookup_time(rate.snap_nearest(filmcraft_time::Tick::from_rational(p, num, den)), num, den, rate, true) == p)+}+ /// A container's sync sample for `i` when leading pictures need the GOP before: `sync` is the /// index's own sync sample, `leads(i, key)` says `i` is shown before `key`. One step per GOP, so a /// loop: a crafted index can chain tens of thousands of GOPs (#2803). A `sync` past `i` stops the@@ -58,6 +58,9 @@ pub struct MkvSource { /// Video codec as an ISO-BMFF sample entry (for the decoder factories). ventry: Option<SampleEntry>, video: GopCache,+ /// Look frames up by the start of their frame: the video track's timestamp unit is half a frame+ /// or longer and its samples sit on the nominal frame grid ([`crate::gop::snap_coarse_units`]).+ snap: bool, /// The alpha layer of a VP8 / VP9 track with `AlphaMode` 1 (WebM transparency, #402). alpha: Option<Arc<crate::webm_alpha::AlphaLayer>>, /// The playable audio tracks in file order: `info.audio_streams[k]` describes `audios[k]`.@@ -221,10 +224,11 @@ fn to_tick(t: &filmcraft_matroska::Track, pts: i64) -> Tick { /// A time as the track timestamp to look a frame up by: rounded to the nearest tick, as muxers /// round frame times to `TimestampScale` (usually 1 ms). Flooring skipped every frame whose-/// timestamp was rounded up (every third frame at 30 fps).-fn from_tick(t: &filmcraft_matroska::Track, time: Tick) -> i64 {+/// timestamp was rounded up (every third frame at 30 fps). See [`crate::gop::lookup_time`] for+/// `snap`, timestamp units as long as a frame.+fn from_tick(t: &filmcraft_matroska::Track, rate: FrameRate, time: Tick, snap: bool) -> i64 { let (n, d) = tb(t);- time.to_rational_round(n, d)+ crate::gop::lookup_time(time, n, d, rate, snap) } /// VP9 configuration from the Matroska `CodecPrivate` feature list (ID / length / value triples:@@ -460,7 +464,12 @@ impl MkvSource { && t.samples.iter().any(|s| s.addition.is_some()) }); let alpha = alpha.map(|t| Arc::new(crate::webm_alpha::AlphaLayer::new(bytes.clone(), t.samples.iter().map(|s| (s.pts, s.addition)))));- Ok(Self { info, bytes, file, vtrack, ventry, video: GopCache::new(explicit_color).with_rotation(rotation), alpha, audios })+ let snap = vtrack.zip(info.video.as_ref()).is_some_and(|(i, v)| {+ let t = &file.tracks[i];+ let (n, d) = tb(t);+ crate::gop::snap_coarse_units(t.samples.iter().map(|s| s.pts), n, d, v.frame_rate)+ });+ Ok(Self { info, bytes, file, vtrack, ventry, video: GopCache::new(explicit_color).with_rotation(rotation), snap, alpha, audios }) } fn read(&self, track: usize, i: usize) -> crate::Result<Vec<u8>> {@@ -616,8 +625,9 @@ impl MediaSource for MkvSource { fn video_frame(&self, req: FrameRequest) -> Result<Arc<VideoFrame>, MediaError> { let ti = self.vtrack.ok_or(MediaError::NoStream("video"))?; let t = req.time.max(Tick::ZERO);- let target = from_tick(&self.file.tracks[ti], t);- let late = filmcraft_media::cancel::catch_up().map(|m| from_tick(&self.file.tracks[ti], t - m));+ let rate = self.info.video.as_ref().map(|v| v.frame_rate).unwrap_or_default();+ let target = from_tick(&self.file.tracks[ti], rate, t, self.snap);+ let late = filmcraft_media::cancel::catch_up().map(|m| from_tick(&self.file.tracks[ti], rate, t - m, self.snap)); Ok(self.video.frame_late(&MkvVideo { src: self, track: ti }, target, late)?) } @@ -53,6 +53,9 @@ pub struct Mp4Source { file: Mp4File, vtrack: Option<usize>, video: GopCache,+ /// Look frames up by the start of their frame: the video track's timestamp unit is half a frame+ /// or longer and its samples sit on the nominal frame grid ([`crate::gop::snap_coarse_units`]).+ snap: bool, /// The playable audio tracks in file order: `info.audio_streams[k]` describes `audios[k]`. audios: Vec<Mp4Audio>, }@@ -368,7 +371,11 @@ impl Mp4Source { .collect(); snap_to_frame_grid(&mut file, vtrack); rebase_unedited_video(&mut file, vtrack, &mut audios);- Ok(Self { info, bytes, file, vtrack, video: GopCache::new(explicit_color).with_rotation(rotation).with_crop(aperture), audios })+ let snap = vtrack.zip(info.video.as_ref()).is_some_and(|(i, v)| {+ let t = &file.tracks[i];+ crate::gop::snap_coarse_units(t.samples.iter().map(|s| s.pts.saturating_add(t.edit_offset)), 1, i64::from(t.timescale), v.frame_rate)+ });+ Ok(Self { info, bytes, file, vtrack, video: GopCache::new(explicit_color).with_rotation(rotation).with_crop(aperture), snap, audios }) } fn read(&self, track: usize, i: usize) -> crate::Result<Vec<u8>> {@@ -378,10 +385,10 @@ impl Mp4Source { fn video_at(&self, t: Tick) -> crate::Result<Arc<VideoFrame>> { let ti = self.vtrack.ok_or_else(|| CodecError::Unsupported("no video".into()))?; let ts = self.file.tracks[ti].timescale as i64;- // Nearest, not floor: with a coarse timescale (1000) sample times are rounded, and a- // floored lookup lands one unit short of every frame whose time was rounded up.- let late = filmcraft_media::cancel::catch_up().map(|m| (t - m).to_rational_round(1, ts));- self.video.frame_late(&Mp4Video { src: self, track: ti }, t.to_rational_round(1, ts), late)+ let rate = self.info.video.as_ref().map(|v| v.frame_rate).unwrap_or_default();+ let at = |t: Tick| crate::gop::lookup_time(t, 1, ts, rate, self.snap);+ let late = filmcraft_media::cancel::catch_up().map(|m| at(t - m));+ self.video.frame_late(&Mp4Video { src: self, track: ti }, at(t), late) } /// Decoded packet `i` of audio stream `a` (`ainfo` is its `AudioStreamInfo`; `st` its decoder state).@@ -174,8 +174,13 @@ fn mp4_with_frames_on_the_grid_shows_every_frame_once() { /// An MP4 at 90 kHz with these sample durations. fn mp4_90k(durations: &[u32]) -> Arc<[u8]> {+ mp4_durations(90_000, durations)+}++/// An MP4 at timescale `ts` with the given sample durations.+fn mp4_durations(ts: u32, durations: &[u32]) -> Arc<[u8]> { let mut w = Mp4Writer::new(Cursor::new(Vec::new()), WriterOptions::new(Brand::Mov)).unwrap();- let t = w.add_track(TrackConfig::new(SampleEntry::jpeg((BITS * BLOCK) as u16, BLOCK as u16), 90_000)).unwrap();+ let t = w.add_track(TrackConfig::new(SampleEntry::jpeg((BITS * BLOCK) as u16, BLOCK as u16), ts)).unwrap(); for (k, &duration) in durations.iter().enumerate() { w.write_sample(t, WriteSample { data: &picture(k), duration, composition_offset: 0, is_sync: true }).unwrap(); }@@ -235,3 +240,121 @@ fn mp4_variable_rate_and_on_grid_frame_times_stay_put() { } assert!(failures.is_empty(), "{}", failures[..failures.len().min(8)].join("\n")); }++/// Asks for every frame at its start, its middle and nine tenths in, expects that frame each time.+fn inside_frame_problems(src: &dyn MediaSource, rate: FrameRate) -> Vec<String> {+ let frame = rate.frame_duration().0;+ let mut wrong = Vec::new();+ for k in 0..frame_count(rate) {+ for tenths in [0, 5, 9] {+ let t = rate.tick_of(k as i64) + Tick(frame / 10 * tenths);+ let got = index_of(&src.video_frame(FrameRequest::full(t)).unwrap());+ if got != k {+ wrong.push(format!("frame {k} + {tenths}/10: got {got}"));+ }+ }+ }+ wrong+}++/// An MP4 whose timescale is `per_frame` units per frame of `rate` (an integer rate), every sample+/// that many units long.+fn mp4_units_per_frame(rate: FrameRate, per_frame: u32) -> Arc<[u8]> {+ let ts = (rate.num / rate.den) as u32 * per_frame;+ let mut w = Mp4Writer::new(Cursor::new(Vec::new()), WriterOptions::new(Brand::Mov)).unwrap();+ let t = w.add_track(TrackConfig::new(SampleEntry::jpeg((BITS * BLOCK) as u16, BLOCK as u16), ts)).unwrap();+ for k in 0..frame_count(rate) {+ w.write_sample(t, WriteSample { data: &picture(k), duration: per_frame, composition_offset: 0, is_sync: true }).unwrap();+ }+ w.finish().unwrap().into_inner().into()+}++/// An MP4 muxed with timescale = frame rate (one unit per frame, as penumbra writes its recordings):+/// half a unit is half a frame, so rounding to the nearest unit showed the next frame from every+/// frame's middle on. Two units per frame had the same problem from three quarters in.+#[test]+fn mp4_with_a_timescale_of_one_or_two_units_per_frame_finds_the_frame_inside_it() {+ let mut failures = Vec::new();+ for rate in [FrameRate::FPS_24, FrameRate::FPS_25, FrameRate::FPS_30, FrameRate::FPS_60] {+ for per_frame in [1, 2] {+ let src = crate::open_bytes("synthetic.mov", mp4_units_per_frame(rate, per_frame)).unwrap();+ for p in inside_frame_problems(src.as_ref(), rate).into_iter().take(3) {+ failures.push(format!("{rate}, {per_frame} units per frame: {p}"));+ }+ }+ }+ assert!(failures.is_empty(), "mov:\n{}", failures.join("\n"));+}++/// A Matroska file whose `TimestampScale` is one frame of `rate` in whole nanoseconds, `scale_ns`+/// (rounded down or up where a frame is no whole number of them).+fn mkv_scale(rate: FrameRate, scale_ns: u64) -> Arc<[u8]> {+ let mut spec = TrackSpec::new(TrackKind::Video, "V_MJPEG");+ spec.video_size = Some((BITS * BLOCK, BLOCK));+ spec.default_duration_ns = Some(scale_ns);+ let opts = MuxOptions { timestamp_scale: scale_ns, ..MuxOptions::default() };+ let mut w = MkvWriter::new(Cursor::new(Vec::new()), vec![spec], opts).unwrap();+ for k in 0..frame_count(rate) {+ let ns = (k as i128 * 1_000_000_000 * rate.den as i128 / rate.num as i128) as i64;+ w.write_frame(0, ns, true, &picture(k), None).unwrap();+ }+ w.finish().unwrap().into_inner().into()+}++/// The same for Matroska with a timestamp unit of one frame: exact at 25 fps, a fraction of a+/// nanosecond short of or past the frame at 24, 30 and 60 fps.+#[test]+fn mkv_with_a_timestamp_unit_of_one_frame_finds_the_frame_inside_it() {+ let mut failures = Vec::new();+ for rate in [FrameRate::FPS_24, FrameRate::FPS_25, FrameRate::FPS_30, FrameRate::FPS_60] {+ let exact = 1_000_000_000 * rate.den as u64;+ let floor = exact / rate.num as u64;+ let ceil = exact.div_ceil(rate.num as u64);+ for scale_ns in if floor == ceil { vec![floor] } else { vec![floor, ceil] } {+ let src = crate::open_bytes("synthetic.mkv", mkv_scale(rate, scale_ns)).unwrap();+ for p in inside_frame_problems(src.as_ref(), rate).into_iter().take(3) {+ failures.push(format!("{rate}, {scale_ns} ns units: {p}"));+ }+ }+ }+ assert!(failures.is_empty(), "mkv:\n{}", failures.join("\n"));+}++/// A Matroska file with `TimestampScale` `scale_ns` and a frame at each of `starts` (units of it).+fn mkv_starts(scale_ns: u64, starts: &[i64]) -> Arc<[u8]> {+ let mut spec = TrackSpec::new(TrackKind::Video, "V_MJPEG");+ spec.video_size = Some((BITS * BLOCK, BLOCK));+ let opts = MuxOptions { timestamp_scale: scale_ns, ..MuxOptions::default() };+ let mut w = MkvWriter::new(Cursor::new(Vec::new()), vec![spec], opts).unwrap();+ for (k, &s) in starts.iter().enumerate() {+ w.write_frame(0, s * scale_ns as i64, true, &picture(k), None).unwrap();+ }+ w.finish().unwrap().into_inner().into()+}++/// A coarse unit is no constant rate: at two units a frame (timescale 50, nominal 25 fps), a track+/// with one sample a unit long has a frame starting between two nominal frames. Moving a request+/// at that start back to its nominal frame would show the frame before; such a track keeps the+/// lookup by the nearest unit, in both containers.+#[test]+fn variable_rate_track_with_a_coarse_unit_keeps_its_off_grid_frame() {+ let durations = [2u32, 1, 2, 2, 2, 2, 2, 2];+ let starts: Vec<i64> = durations.iter().scan(0i64, |t, &d| Some(std::mem::replace(t, *t + i64::from(d)))).collect();+ let sources = [+ ("mov", crate::open_bytes("synthetic.mov", mp4_durations(50, &durations)).unwrap()),+ ("mkv", crate::open_bytes("synthetic.mkv", mkv_starts(20_000_000, &starts)).unwrap()),+ ];+ let mut failures = Vec::new();+ for (name, src) in &sources {+ assert_eq!(src.info().video.as_ref().unwrap().frame_rate, FrameRate::FPS_25, "{name}: nominal rate");+ // every unit up to the last frame: the last frame starting at or before it+ for t in 0..=*starts.last().unwrap() {+ let want = starts.iter().rposition(|&s| s <= t).unwrap();+ let got = index_of(&src.video_frame(FrameRequest::full(Tick::from_rational(t, 1, 50))).unwrap());+ if got != want {+ failures.push(format!("{name}: at {t}/50 s wanted frame {want}, got {got}"));+ }+ }+ }+ assert!(failures.is_empty(), "{}", failures.join("\n"));+}