set pts in ffmpeg screen encoder #1020
Conversation
WalkthroughAdds start_time-aware PTS alignment to studio-mode capture pipelines by introducing a start_time parameter, computing elapsed durations, and converting them to PTS via a new H264Encoder::get_pts. Adjusts H.264 encoder time_base usage, updates Direct3D paths (macOS/Windows) to set frame PTS, wires start_time through studio_recording, and updates a recording example import. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SR as StudioRecording
participant CP as CapturePipeline (studio mode)
participant D3D as Direct3DCapture
participant ENC as H264Encoder
participant FF as FFmpeg Frame
Note over SR,CP: Pipeline creation with start_time (Timestamps)
SR->>CP: make_studio_mode_pipeline(..., start_time)
CP->>D3D: initialize(start_time)
loop For each captured frame
D3D->>D3D: Compute elapsed = (ts - first_ts) aligned to start_time
D3D->>ENC: get_pts(elapsed)
ENC-->>D3D: pts (i64)
D3D->>FF: set pts
Note right of FF: PTS aligned to start_time
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/recording/src/studio_recording.rs (1)
808-813: Avoid underflow on Duration math and unify PTS roundingSubtracting Durations can underflow if a late/OOO frame arrives; some blocks round PTS, others truncate. Guard with checked_sub and round consistently.
Mic:
- let elapsed = timestamp.duration_since(start_time) - - first_timestamp.duration_since(start_time); - frame.set_pts(Some((elapsed.as_secs_f64() * rate) as i64)); + if let Some(elapsed) = timestamp + .duration_since(start_time) + .checked_sub(first_timestamp.duration_since(start_time)) + { + frame.set_pts(Some((elapsed.as_secs_f64() * rate).round() as i64)); + } else { + continue; + }System audio:
- let elapsed = timestamp.duration_since(start_time) - - first_timestamp.duration_since(start_time); - frame.set_pts(Some((elapsed.as_secs_f64() * rate).round() as i64)); + if let Some(elapsed) = timestamp + .duration_since(start_time) + .checked_sub(first_timestamp.duration_since(start_time)) + { + frame.set_pts(Some((elapsed.as_secs_f64() * rate).round() as i64)); + } else { + continue; + }Camera:
- let elapsed = timestamp.duration_since(start_time) - - first_timestamp.duration_since(start_time); - frame.set_pts(Some((elapsed.as_secs_f64() * rate) as i64)); + if let Some(elapsed) = timestamp + .duration_since(start_time) + .checked_sub(first_timestamp.duration_since(start_time)) + { + frame.set_pts(Some((elapsed.as_secs_f64() * rate).round() as i64)); + } else { + continue; + }Also applies to: 863-866, 916-919
crates/recording/src/capture_pipeline.rs (1)
323-325: Harden Windows software-encoder path: no unwrap, prevent Duration underflow, avoid shadowing
- Avoid panic on oneshot send.
- Use checked_sub to prevent underflow if timestamps regress.
- Don’t shadow first_timestamp with a different type; improves clarity.
- timestamp_tx.send(timestamp).unwrap(); + let _ = timestamp_tx.send(timestamp);- let mut first_timestamp = None; + let mut first_timestamp: Option<Timestamp> = None; @@ - let first_timestamp = first_timestamp.get_or_insert(timestamp); + let first_ts = first_timestamp.get_or_insert(timestamp); @@ - let elapsed = timestamp.duration_since(start_time) - - first_timestamp.duration_since(start_time); - ff_frame.set_pts(Some(encoder.get_pts(elapsed))); + if let Some(elapsed) = timestamp + .duration_since(start_time) + .checked_sub(first_ts.duration_since(start_time)) + { + ff_frame.set_pts(Some(encoder.get_pts(elapsed))); + } else { + continue; + }Also applies to: 355-375
🧹 Nitpick comments (3)
crates/enc-ffmpeg/src/video/h264.rs (1)
126-127: Make stream time_base explicit (or drop the dead constant)Relying on muxer defaults can cause subtle PTS rescale issues; also TIME_BASE is unused.
- // output_stream.set_time_base((1, H264Encoder::TIME_BASE)); + // Make stream time_base explicit to avoid muxer defaults differing from encoder PTS. + output_stream.set_time_base(input_config.time_base);- const TIME_BASE: i32 = 15000; + // removed unused TIME_BASEAlso applies to: 152-153
crates/recording/src/studio_recording.rs (1)
468-471: Prefer tracing over println! for consistencyUse tracing::{info} to match the rest of the module’s logging.
- println!("recording successfully stopped"); + tracing::info!("recording successfully stopped");crates/recording/src/capture_pipeline.rs (1)
50-59: macOS studio path ignores start_timeUnderscoring avoids warnings, but this leaves PTS alignment asymmetric vs Windows. Confirm AVFoundation path doesn’t require start_time for A/V sync, or plan follow‑up to propagate it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/enc-ffmpeg/src/video/h264.rs(5 hunks)crates/recording/examples/recording-cli.rs(1 hunks)crates/recording/src/capture_pipeline.rs(6 hunks)crates/recording/src/studio_recording.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{crates,apps/cli,apps/desktop/src-tauri}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
{crates,apps/cli,apps/desktop/src-tauri}/**/*.rs: Rust code must be formatted with rustfmt and adhere to workspace clippy lints
Rust module files should use snake_case names
Files:
crates/recording/examples/recording-cli.rscrates/enc-ffmpeg/src/video/h264.rscrates/recording/src/studio_recording.rscrates/recording/src/capture_pipeline.rs
{crates,apps/cli,apps/desktop/src-tauri}/**/{src,tests}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Rust tests should live in src (inline/unit) or tests (integration) directories
Files:
crates/enc-ffmpeg/src/video/h264.rscrates/recording/src/studio_recording.rscrates/recording/src/capture_pipeline.rs
🧬 Code graph analysis (2)
crates/recording/examples/recording-cli.rs (2)
crates/recording/src/studio_recording.rs (1)
builder(129-131)crates/recording/src/instant_recording.rs (1)
builder(154-156)
crates/recording/src/studio_recording.rs (1)
crates/video-decode/src/ffmpeg.rs (1)
start_time(98-100)
⏰ 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). (4)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
crates/enc-ffmpeg/src/video/h264.rs (1)
108-110: Confirm encoder time_base sourceSwitching to input_config.time_base is fine if all queued frame PTS are computed in the same base (they are via get_pts). Please confirm VideoInfo::time_base matches capture timestamps end‑to‑end (Windows + macOS), or document the assumption.
crates/recording/examples/recording-cli.rs (1)
59-71: Switch to studio_recording::Actor looks goodCall site change aligns with the studio‑mode pipeline. No issues spotted.
crates/recording/src/studio_recording.rs (1)
751-757: Start-time propagation into screen pipeline: LGTMPassing start_time into make_studio_mode_pipeline is correct and consistent with the rest of the pipeline.
crates/recording/src/capture_pipeline.rs (2)
285-287: Explicit tracing on native-encoder fallback: LGTMClearer diagnostics; good change.
21-31: API change: in-repo impls & callers updated — no action requiredAll MakeCapturePipeline impls and the call site in this repo were updated to accept the new start_time parameter:
- crates/recording/src/capture_pipeline.rs — impl for screen_capture::CMSampleBufferCapture: fn make_studio_mode_pipeline(..., _start_time: Timestamps).
- crates/recording/src/capture_pipeline.rs — impl for screen_capture::Direct3DCapture: fn make_studio_mode_pipeline(..., start_time: Timestamps).
- crates/recording/src/studio_recording.rs — call passes start_time into ScreenCaptureMethod::make_studio_mode_pipeline (around line ~751).
| pub fn get_pts(&self, duration: Duration) -> i64 { | ||
| (duration.as_secs_f32() * self.config.time_base.denominator() as f32 | ||
| / self.config.time_base.numerator() as f32) as i64 | ||
| } |
There was a problem hiding this comment.
Fix PTS conversion precision and rounding
Using f32 loses precision and truncation can drift PTS over long runs. Use f64 and round to nearest tick.
- pub fn get_pts(&self, duration: Duration) -> i64 {
- (duration.as_secs_f32() * self.config.time_base.denominator() as f32
- / self.config.time_base.numerator() as f32) as i64
- }
+ pub fn get_pts(&self, duration: Duration) -> i64 {
+ let tb = self.config.time_base;
+ (duration.as_secs_f64() * tb.denominator() as f64 / tb.numerator() as f64).round() as i64
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn get_pts(&self, duration: Duration) -> i64 { | |
| (duration.as_secs_f32() * self.config.time_base.denominator() as f32 | |
| / self.config.time_base.numerator() as f32) as i64 | |
| } | |
| pub fn get_pts(&self, duration: Duration) -> i64 { | |
| let tb = self.config.time_base; | |
| (duration.as_secs_f64() * tb.denominator() as f64 / tb.numerator() as f64).round() as i64 | |
| } |
🤖 Prompt for AI Agents
In crates/enc-ffmpeg/src/video/h264.rs around lines 211 to 214, the PTS
conversion currently uses f32 and a truncating cast which loses precision and
causes drift; change the computation to use f64 (use duration.as_secs_f64()),
cast the time_base numerator/denominator to f64, perform the division and
multiplication in f64, round the resulting value to the nearest integer (e.g.
.round()) and then cast to i64 to return the final PTS value.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements