Skip to content

speed up macOS scrubbing with stricter decoder reuse#1785

Merged
richiemcilroy merged 3 commits intomainfrom
playback-optimisations
May 7, 2026
Merged

speed up macOS scrubbing with stricter decoder reuse#1785
richiemcilroy merged 3 commits intomainfrom
playback-optimisations

Conversation

@richiemcilroy
Copy link
Copy Markdown
Member

@richiemcilroy richiemcilroy commented May 7, 2026

narrows decoder reuse during scrubbing on macOS AVAssetReader by routing scrub requests through a configurable reuse threshold on the multi-position decoder pool (default playback behavior unchanged).

Greptile Summary

This PR speeds up macOS AVAssetReader scrubbing by routing scrub requests through a tighter 0.5 s decoder-reuse window (vs. the 5–10 s window used during linear playback). When no pooled decoder is within 0.5 s of the requested position, the nearest decoder is reset to the target keyframe instead of being walked forward through many frames sequentially.

  • multi_position.rs: find_best_decoder_for_time is refactored to delegate to a new find_best_decoder_for_time_with_reuse_threshold; callers can pass a custom threshold that is clamped to the pool's configured playback threshold, so no out-of-bounds value is possible.
  • avassetreader.rs: select_best_decoder gains an is_scrubbing parameter; when true it calls the new threshold variant with SCRUB_REUSE_THRESHOLD_SECS = 0.5, leaving linear playback behavior unchanged.
  • Benchmarks in PLAYBACK-FINDINGS.md show scrub decode p95 dropping from 231 ms to 47 ms (−79 %) with no regression to linear playback or A/V sync.

Confidence Score: 4/5

Safe to merge; the change is isolated to the scrub code path and the linear playback path is demonstrably unaffected.

The refactoring is clean and the threshold clamp prevents callers from accidentally widening the scrub window beyond the pool's playback threshold. The only gap is that the new test covers only the forced-reset case; a complementary test for the reuse case would make the coverage complete, but its absence does not indicate a defect in the shipped logic.

The test block at the bottom of multi_position.rs would benefit from a second test covering the complementary reuse case.

Important Files Changed

Filename Overview
crates/rendering/src/decoder/multi_position.rs Refactors find_best_decoder_for_time to delegate to a new find_best_decoder_for_time_with_reuse_threshold method; adds a test covering the forced-reset path when the decoder is beyond the 0.5s window.
crates/rendering/src/decoder/avassetreader.rs Adds SCRUB_REUSE_THRESHOLD_SECS = 0.5 and threads is_scrubbing into select_best_decoder, routing scrub calls through the tighter threshold; call site correctly supplies the scrub-detector's flag.
crates/editor/PLAYBACK-FINDINGS.md Documentation-only update recording the 2026-05-07 scrub optimization session with baseline and final benchmark numbers.
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
crates/rendering/src/decoder/multi_position.rs:418-421
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);
    }
}
```

Reviews (1): Last reviewed commit: "docs(editor): record scrub benchmark res..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

@brin-security-scanner brin-security-scanner Bot added contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis. labels May 7, 2026
@paragon-review
Copy link
Copy Markdown

paragon-review Bot commented May 7, 2026

Paragon Review Skipped

Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review.

Please visit https://app.paragon.run to finish your review.

Comment on lines +418 to 421
assert_eq!(decoder_id, 1);
assert!(needs_reset);
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Suggested change
assert_eq!(decoder_id, 1);
assert!(needs_reset);
}
}
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);
}
}
Prompt To Fix With AI
This 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.

)
}

pub fn find_best_decoder_for_time_with_reuse_threshold(
Copy link
Copy Markdown

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-rendering right now; making it crate-private keeps the surface area smaller.

Suggested change
pub fn find_best_decoder_for_time_with_reuse_threshold(
pub(crate) fn find_best_decoder_for_time_with_reuse_threshold(

@richiemcilroy richiemcilroy merged commit 5748afd into main May 7, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:verified Contributor passed trust analysis. pr:verified PR passed security analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant