-
Notifications
You must be signed in to change notification settings - Fork 1.5k
speed up macOS scrubbing with stricter decoder reuse #1785
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8b99691
4fe80ac
876802b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -135,6 +135,19 @@ impl DecoderPoolManager { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| &mut self, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| requested_time: f32, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| decoder_count: usize, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> (usize, f32, bool) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.find_best_decoder_for_time_with_reuse_threshold( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| requested_time, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| decoder_count, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.reposition_threshold, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn find_best_decoder_for_time_with_reuse_threshold( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| &mut self, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| requested_time: f32, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| decoder_count: usize, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| reuse_threshold: f32, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> (usize, f32, bool) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.total_accesses += 1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -150,6 +163,7 @@ impl DecoderPoolManager { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut best_decoder_id = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut best_distance = f32::MAX; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut needs_reset = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let reuse_threshold = reuse_threshold.clamp(0.0, self.reposition_threshold); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if decoder_count == 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return (0, f32::MAX, true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -158,7 +172,7 @@ impl DecoderPoolManager { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for position in self.positions.iter().filter(|p| p.id < decoder_count) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let distance = (position.position_secs - requested_time).abs(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let is_usable = position.position_secs <= requested_time | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| && (requested_time - position.position_secs) < self.reposition_threshold; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| && (requested_time - position.position_secs) < reuse_threshold; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if is_usable && distance < best_distance { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| best_distance = distance; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -320,6 +334,7 @@ impl Default for ScrubDetector { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[cfg(test)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mod tests { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use super::*; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::path::PathBuf; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn test_calculate_optimal_pool_size_short_video() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -384,4 +399,23 @@ mod tests { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(calculate_reposition_threshold(duration_55_min), 10.0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn test_custom_reuse_threshold_forces_reset_when_decoder_is_too_far_behind() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let runtime = tokio::runtime::Runtime::new().unwrap(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let config = MultiPositionDecoderConfig { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| path: PathBuf::from("fixture.mp4"), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tokio_handle: runtime.handle().clone(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| keyframe_index: None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fps: 60, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| duration_secs: 20.0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut manager = DecoderPoolManager::new(config); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let (decoder_id, _, needs_reset) = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| manager.find_best_decoder_for_time_with_reuse_threshold(6.0, 5, 0.5); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert_eq!(decoder_id, 1); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert!(needs_reset); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+418
to
421
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/rendering/src/decoder/multi_position.rs
Line: 418-421
Comment:
The new test only covers the "forces reset" path (decoder too far behind). There is no complementary test verifying that a decoder positioned within the 0.5 s window is actually reused (`needs_reset == false`). Without that case, a regression that accidentally always resets would still pass the test suite.
```suggestion
assert_eq!(decoder_id, 1);
assert!(needs_reset);
}
#[test]
fn test_custom_reuse_threshold_reuses_decoder_when_within_window() {
let runtime = tokio::runtime::Runtime::new().unwrap();
let config = MultiPositionDecoderConfig {
path: PathBuf::from("fixture.mp4"),
tokio_handle: runtime.handle().clone(),
keyframe_index: None,
fps: 60,
duration_secs: 20.0,
};
let mut manager = DecoderPoolManager::new(config);
// Advance decoder 0 to 5.8 s so it sits within the 0.5 s window of 6.0 s.
manager.update_decoder_position(0, 5.8);
let (_, _, needs_reset) =
manager.find_best_decoder_for_time_with_reuse_threshold(6.0, 5, 0.5);
assert!(!needs_reset);
}
}
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it’s only used within
cap-renderingright now; making it crate-private keeps the surface area smaller.