Stabilize editor playback pacing and validate fps #1245
Conversation
WalkthroughThe playback system adds FPS validation during startup with explicit error handling, returning early on invalid FPS. The playback loop is refactored from time-based calculations to a frame-driven scheduling model using tokio::select! to synchronize frame progression with a computed deadline. Changes
Sequence DiagramsequenceDiagram
participant Editor
participant Playback
participant Timing
Editor->>Playback: start(fps, resolution)
rect rgb(200, 220, 255)
Note over Playback: Validation Phase
Playback->>Playback: Convert fps to f64
Playback->>Playback: Check finite & > 0
alt Invalid FPS
Playback-->>Editor: Err(InvalidFps)
Editor->>Editor: Log warning, abort
else Valid FPS
Playback->>Timing: Calculate frame_duration
Playback->>Timing: Initialize next_deadline
end
end
rect rgb(220, 240, 220)
Note over Playback: Frame-Driven Loop
loop Per Frame
Playback->>Timing: tokio::select! {<br/>stop_signal | deadline}
alt Deadline Reached
Playback->>Playback: Increment frame_number
Playback->>Playback: Calculate playback_time<br/>(frame_number / fps)
Playback->>Playback: Fetch segment by playback_time
Playback->>Playback: Render frame
Playback->>Timing: Compute next_deadline
else Stop Signal Received
break Exit loop
end
end
end
end
Playback-->>Editor: Ok(PlaybackHandle)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes The review requires understanding a significant refactoring of timing-critical code from time-based to frame-driven scheduling, including new FPS validation logic, error propagation changes, and frame synchronization mechanisms that affect playback accuracy and behavior. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
richiemcilroy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/editor/src/playback.rs (2)
23-26: LGTM! Simple error type for FPS validation.The error enum is appropriate for internal error handling. Optionally, consider implementing
std::error::ErrorandDisplayif this error needs to be propagated or formatted in user-facing contexts.
55-60: Simplify the FPS validation check.Since
fpsis au32, converting it tof64will always produce a finite value. Theis_finite()check is redundant. The real concern is whetherfpsis zero.Apply this diff to simplify the validation:
- let fps_f64 = fps as f64; - - if !(fps_f64.is_finite() && fps_f64 > 0.0) { + if fps == 0 { warn!(fps, "Invalid FPS provided for playback start"); return Err(PlaybackStartError::InvalidFps); } + + let fps_f64 = fps as f64;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/editor/src/editor_instance.rs(3 hunks)crates/editor/src/playback.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/editor/src/editor_instance.rscrates/editor/src/playback.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/editor/src/editor_instance.rscrates/editor/src/playback.rs
🧬 Code graph analysis (1)
crates/editor/src/playback.rs (3)
crates/media-info/src/lib.rs (1)
fps(221-223)apps/desktop/src-tauri/src/export.rs (1)
fps(17-22)crates/rendering/src/project_recordings.rs (1)
fps(50-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
crates/editor/src/editor_instance.rs (2)
2-2: LGTM! Import additions support the error handling.The new imports enable explicit error pattern matching and warning logs for invalid FPS scenarios.
Also applies to: 13-13
148-163: LGTM! Proper error handling for invalid FPS.The error handling correctly matches on the Result type, logs a warning for invalid FPS, and aborts the playback start via early return. This prevents invalid playback from proceeding.
crates/editor/src/playback.rs (6)
91-96: LGTM! Frame-driven scheduling improves timing precision.Calculating the deadline from the frame offset relative to start time prevents accumulating timing errors, which is superior to repeatedly adding frame durations.
98-105: LGTM! Proper synchronization for frame timing and cancellation.The
tokio::select!correctly prioritizes the stop signal, and the subsequent stop check at line 103 is good defensive programming to catch signals that arrive as the sleep completes.
107-117: LGTM! Clean playback time calculation and segment retrieval.The frame-based playback time calculation is correct, and the let-else pattern provides clean error handling for segment retrieval.
119-132: LGTM! Proper clip offset retrieval and cancellable frame fetching.The clip offset lookup with fallback to default is correct, and using
tokio::select!to make frame retrieval cancellable ensures responsive shutdown.
134-159: LGTM! Rendering logic with frame catch-up for drift prevention.The rendering is correctly conditioned on frame availability. The catch-up logic (lines 154-159) ensures the playback doesn't fall behind by skipping frames when rendering is slow—appropriate behavior for real-time playback to maintain synchronization.
167-167: LGTM! Correct return type.The
Ok(handle)return matches the newResultsignature.
1 participant
This pull request introduces improved error handling, performance and timing precision for video playback in the editor. The main changes include adding a new error type to handle invalid FPS values, updating the playback start logic to return a result instead of panicking, and refining the frame scheduling to ensure smoother and more accurate playback.
Improves performance of playback in the editor by tightening the playback loop so we only render once per frame interval instead of hammering the decoders continuously.
Summary by CodeRabbit