From bf5ab47cdebcfeb70ea403fd889ef525cb614dba Mon Sep 17 00:00:00 2001 From: Richie McIlroy <33632126+richiemcilroy@users.noreply.github.com> Date: Thu, 15 Oct 2026 06:17:13 +0900 Subject: [PATCH 1/3] perf(video-decode): start AVAssetReader seeks on the exact keyframe tick --- crates/video-decode/src/avassetreader.rs | 68 ++++++++++++++++++------ 1 file changed, 53 insertions(+), 15 deletions(-) diff --git a/crates/video-decode/src/avassetreader.rs b/crates/video-decode/src/avassetreader.rs index 1f2dfc7b02a..e610a3164af 100644 --- a/crates/video-decode/src/avassetreader.rs +++ b/crates/video-decode/src/avassetreader.rs @@ -14,6 +14,7 @@ use tokio::runtime::Handle as TokioHandle; #[derive(Clone)] pub struct KeyframeIndex { keyframes: Vec<(u32, f64)>, + time_base: ffmpeg::Rational, fps: f64, duration_secs: f64, pixel_format: Option, @@ -271,6 +272,7 @@ impl KeyframeIndex { Ok(Self { keyframes, + time_base, fps, duration_secs, pixel_format, @@ -374,20 +376,44 @@ impl KeyframeIndex { pub fn cached_video_info(&self) -> Option<(cv::PixelFormat, u32, u32)> { self.pixel_format.map(|pf| (pf, self.width, self.height)) } -} -fn compute_seek_time(keyframe_index: Option<&Arc>, requested_time: f32) -> f32 { - if let Some(kf_index) = keyframe_index { - let keyframes = kf_index.keyframes(); - // Rounded frame numbers can alias keyframes across a VFR gap and seek - // past the image that should still be held at the requested time. - let after_request = - keyframes.partition_point(|(_, time)| *time <= f64::from(requested_time)); - if let Some((_, keyframe_time)) = keyframes.get(after_request.saturating_sub(1)) { - return *keyframe_time as f32; + // The reader decodes from the sync sample at or before its start time, so + // a start rounded to just below a keyframe decodes the whole previous GOP + // and vends its last frame retimed to the start. Starting on the exact + // stream tick keeps the decode on the requested keyframe. + fn exact_time(&self, secs: f64) -> cm::Time { + let numerator = i64::from(self.time_base.numerator()); + let denominator = self.time_base.denominator(); + if numerator <= 0 || denominator <= 0 { + return cm::Time::with_secs(secs, 100); } + let ticks = (secs * f64::from(denominator) / numerator as f64).round() as i64; + cm::Time::new(ticks * numerator, denominator) } - requested_time +} + +fn seek_keyframe_time(keyframe_index: &KeyframeIndex, requested_time: f32) -> Option { + let keyframes = keyframe_index.keyframes(); + // Rounded frame numbers can alias keyframes across a VFR gap and seek + // past the image that should still be held at the requested time. + let after_request = keyframes.partition_point(|(_, time)| *time <= f64::from(requested_time)); + keyframes + .get(after_request.saturating_sub(1)) + .map(|(_, time)| *time) +} + +fn compute_seek_time(keyframe_index: Option<&Arc>, requested_time: f32) -> f32 { + keyframe_index + .and_then(|index| seek_keyframe_time(index, requested_time)) + .map_or(requested_time, |time| time as f32) +} + +fn reader_start_time(keyframe_index: Option<&Arc>, requested_time: f32) -> cm::Time { + keyframe_index + .and_then(|index| { + seek_keyframe_time(index, requested_time).map(|time| index.exact_time(time)) + }) + .unwrap_or_else(|| cm::Time::with_secs(requested_time as f64, 100)) } pub struct AVAssetReaderDecoder { @@ -464,7 +490,7 @@ impl AVAssetReaderDecoder { let (track_output, reader) = Self::get_reader_track_output( &path, - seek_time, + reader_start_time(keyframe_index.as_ref(), start_time), &tokio_handle, pixel_format, width, @@ -491,7 +517,7 @@ impl AVAssetReaderDecoder { (self.track_output, self.reader) = Self::get_reader_track_output( &self.path, - seek_time, + reader_start_time(self.keyframe_index.as_ref(), requested_time), &self.tokio_handle, self.pixel_format, self.width, @@ -533,7 +559,7 @@ impl AVAssetReaderDecoder { fn get_reader_track_output( path: &Path, - time: f32, + start: cm::Time, handle: &TokioHandle, pixel_format: cv::PixelFormat, width: u32, @@ -553,7 +579,7 @@ impl AVAssetReaderDecoder { .map_err(|e| format!("AssetReader::with_asset: {e}"))?; let time_range = cm::TimeRange { - start: cm::Time::with_secs(time as f64, 100), + start, duration: cm::Time::infinity(), }; @@ -664,6 +690,7 @@ mod tests { fn seek_index(keyframes: Keyframes) -> Arc { Arc::new(KeyframeIndex { keyframes, + time_base: ffmpeg::Rational::new(1, 600), fps: 2.0, duration_secs: 8.0, pixel_format: None, @@ -696,6 +723,17 @@ mod tests { assert_eq!(compute_seek_time(None, 3.0), 3.0); } + #[test] + fn reader_starts_on_the_exact_keyframe_tick() { + let index = seek_index(vec![(0, 0.0), (1, 577.0 / 600.0), (3, 1147.0 / 600.0)]); + let start = reader_start_time(Some(&index), 1.5); + assert_eq!((start.value, start.scale), (577, 600)); + let start = reader_start_time(Some(&index), 1.92); + assert_eq!((start.value, start.scale), (1147, 600)); + let start = reader_start_time(None, 1.5); + assert_eq!((start.value, start.scale), (150, 100)); + } + fn both_indexes(path: &Path) -> Option<(Option, Keyframes)> { ffmpeg::init().unwrap(); let mut input = avformat::input(path).ok()?; From 34a40f1ae855006b5e60962818e886367d53573f Mon Sep 17 00:00:00 2001 From: Richie McIlroy <33632126+richiemcilroy@users.noreply.github.com> Date: Thu, 15 Oct 2026 06:17:13 +0900 Subject: [PATCH 2/3] fix(rendering): reset a reused decoder instead of answering with a frame from an older seek --- crates/rendering/src/decoder/avassetreader.rs | 82 ++++++++++++++++++- 1 file changed, 80 insertions(+), 2 deletions(-) diff --git a/crates/rendering/src/decoder/avassetreader.rs b/crates/rendering/src/decoder/avassetreader.rs index af79c9560e2..a000d973ac8 100644 --- a/crates/rendering/src/decoder/avassetreader.rs +++ b/crates/rendering/src/decoder/avassetreader.rs @@ -380,6 +380,8 @@ struct DecoderInstance { /// Lives on the instance because the reader vends in pts order between /// resets, but request batches may be served by different pool decoders. prev_vended: Option, + /// First frame of the uninterrupted vend run that ends at `prev_vended`. + run_start: Option, _permit: OwnedSemaphorePermit, } @@ -405,6 +407,7 @@ impl DecoderInstance { tokio_handle, keyframe_index, prev_vended: None, + run_start: None, _permit: permit, }) } @@ -416,6 +419,7 @@ impl DecoderInstance { self.frames_iter_valid = true; self.health.reset_counters(); self.prev_vended = None; + self.run_start = None; } Err(e) => { tracing::error!( @@ -448,12 +452,22 @@ impl DecoderInstance { self.frames_iter_valid = true; self.health = DecoderHealth::new(); self.prev_vended = None; + self.run_start = None; Ok(()) } fn current_position(&self) -> f32 { self.inner.current_position_secs() } + + fn would_serve_stale_frame(&self, frame: u32, cache: &BTreeMap) -> bool { + reuse_would_serve_stale_frame( + self.run_start, + self.prev_vended, + cache.range(..=frame).next_back().map(|(&cached, _)| cached), + frame, + ) + } } pub struct AVAssetReaderDecoder { @@ -537,7 +551,12 @@ impl AVAssetReaderDecoder { }) } - fn select_best_decoder(&mut self, requested_time: f32, is_scrubbing: bool) -> (usize, bool) { + fn select_best_decoder( + &mut self, + requested_time: f32, + is_scrubbing: bool, + must_reset: impl Fn(&DecoderInstance) -> bool, + ) -> (usize, bool) { let decoder_count = self.decoders.len(); let (best_id, _distance, needs_reset) = if is_scrubbing { self.pool_manager @@ -552,6 +571,7 @@ impl AVAssetReaderDecoder { }; let decoder_idx = best_id.min(decoder_count.saturating_sub(1)); + let needs_reset = needs_reset || must_reset(&self.decoders[decoder_idx]); let max_decoder_count = self .pool_manager @@ -890,7 +910,10 @@ impl AVAssetReaderDecoder { } let this = this.as_mut().expect("Video reader initialized above"); - let (decoder_idx, was_reset) = this.select_best_decoder(requested_time, is_scrubbing); + let (decoder_idx, was_reset) = + this.select_best_decoder(requested_time, is_scrubbing, |decoder| { + decoder.would_serve_stale_frame(requested_frame, &cache) + }); let cache_min = if was_reset { min_requested_frame.saturating_sub(FRAME_CACHE_SIZE as u32 * 2) @@ -955,6 +978,9 @@ impl AVAssetReaderDecoder { { record_pts_hole(&mut pts_holes, prev, current_frame); } + if decoder.prev_vended.is_none() { + decoder.run_start = Some(current_frame); + } decoder.prev_vended = Some(current_frame); let position_secs = current_frame as f32 / fps as f32; @@ -1278,6 +1304,27 @@ impl AVAssetReaderDecoder { } } +// Requests the reader has already passed, or that fall in a gap before its +// next frame, are answered with the newest cached frame at or before them. +// That frame is only trustworthy when this reader vended it in its current +// run; an older one comes from an unrelated seek and shows stale content. +// With nothing cached, a gap right after an evicted last frame would be +// answered with the following frame instead of the one on screen. +fn reuse_would_serve_stale_frame( + run_start: Option, + prev_vended: Option, + newest_cached_at_or_before: Option, + frame: u32, +) -> bool { + let (Some(run_start), Some(vended)) = (run_start, prev_vended) else { + return false; + }; + match newest_cached_at_or_before { + Some(cached) => cached < run_start, + None => vended < frame, + } +} + fn should_grow_decoder_pool( is_scrubbing: bool, needs_reset: bool, @@ -1357,6 +1404,37 @@ mod tests { )); } + #[test] + fn reused_readers_never_answer_with_frames_from_an_unrelated_seek() { + assert!(!reuse_would_serve_stale_frame(None, None, Some(10), 50)); + assert!(!reuse_would_serve_stale_frame( + Some(40), + Some(55), + Some(48), + 50 + )); + assert!(!reuse_would_serve_stale_frame( + Some(40), + Some(55), + Some(40), + 50 + )); + assert!(reuse_would_serve_stale_frame( + Some(40), + Some(55), + Some(39), + 50 + )); + assert!(reuse_would_serve_stale_frame( + Some(40), + Some(55), + Some(10), + 50 + )); + assert!(!reuse_would_serve_stale_frame(Some(40), Some(55), None, 50)); + assert!(reuse_would_serve_stale_frame(Some(40), Some(48), None, 50)); + } + #[test] fn decoder_pool_grows_only_for_unserved_non_sequential_access() { assert!(should_grow_decoder_pool(true, true, 1, 5)); From 18f8d4035e3d45cc598785991c515e05e28b33ab Mon Sep 17 00:00:00 2001 From: Richie McIlroy <33632126+richiemcilroy@users.noreply.github.com> Date: Thu, 15 Oct 2026 07:39:55 +0900 Subject: [PATCH 3/3] fix(rendering): reset a reused decoder when nothing at or before the request is cached --- crates/rendering/src/decoder/avassetreader.rs | 54 ++++++++----------- 1 file changed, 22 insertions(+), 32 deletions(-) diff --git a/crates/rendering/src/decoder/avassetreader.rs b/crates/rendering/src/decoder/avassetreader.rs index a000d973ac8..2204331d781 100644 --- a/crates/rendering/src/decoder/avassetreader.rs +++ b/crates/rendering/src/decoder/avassetreader.rs @@ -465,7 +465,6 @@ impl DecoderInstance { self.run_start, self.prev_vended, cache.range(..=frame).next_back().map(|(&cached, _)| cached), - frame, ) } } @@ -1308,21 +1307,18 @@ impl AVAssetReaderDecoder { // next frame, are answered with the newest cached frame at or before them. // That frame is only trustworthy when this reader vended it in its current // run; an older one comes from an unrelated seek and shows stale content. -// With nothing cached, a gap right after an evicted last frame would be -// answered with the following frame instead of the one on screen. +// With nothing cached at or before the request, whether the reader has +// passed it or a gap follows its evicted last frame, the only answer left is +// a later frame instead of the one on screen. fn reuse_would_serve_stale_frame( run_start: Option, prev_vended: Option, newest_cached_at_or_before: Option, - frame: u32, ) -> bool { - let (Some(run_start), Some(vended)) = (run_start, prev_vended) else { + let (Some(run_start), Some(_)) = (run_start, prev_vended) else { return false; }; - match newest_cached_at_or_before { - Some(cached) => cached < run_start, - None => vended < frame, - } + newest_cached_at_or_before.is_none_or(|cached| cached < run_start) } fn should_grow_decoder_pool( @@ -1406,33 +1402,27 @@ mod tests { #[test] fn reused_readers_never_answer_with_frames_from_an_unrelated_seek() { - assert!(!reuse_would_serve_stale_frame(None, None, Some(10), 50)); - assert!(!reuse_would_serve_stale_frame( - Some(40), - Some(55), - Some(48), - 50 - )); + assert!(!reuse_would_serve_stale_frame(None, None, Some(10))); + assert!(!reuse_would_serve_stale_frame(Some(40), Some(55), Some(48))); + assert!(!reuse_would_serve_stale_frame(Some(40), Some(55), Some(40))); + assert!(reuse_would_serve_stale_frame(Some(40), Some(55), Some(39))); + assert!(reuse_would_serve_stale_frame(Some(40), Some(55), Some(10))); + assert!(reuse_would_serve_stale_frame(Some(40), Some(48), None)); + } + + #[test] + fn returning_to_a_passed_hold_after_its_frame_is_evicted_resets_the_reader() { + let (run_start, held, vended) = (40, 48, 55); assert!(!reuse_would_serve_stale_frame( - Some(40), - Some(55), - Some(40), - 50 - )); - assert!(reuse_would_serve_stale_frame( - Some(40), - Some(55), - Some(39), - 50 + Some(run_start), + Some(vended), + Some(held) )); assert!(reuse_would_serve_stale_frame( - Some(40), - Some(55), - Some(10), - 50 + Some(run_start), + Some(vended), + None )); - assert!(!reuse_would_serve_stale_frame(Some(40), Some(55), None, 50)); - assert!(reuse_would_serve_stale_frame(Some(40), Some(48), None, 50)); } #[test]