Fix windows compilation because of timestamp typo #176
Conversation
WalkthroughSwapped timestamp source for elapsed/display time in Windows capturer; added a public re-export Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Potential review focus:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/capturer/engine/win/mod.rs (1)
79-86: Fix time-unit mismatch (100ns TimeSpan vs QPC counts) in elapsed/display_time.
frame.timestamp().Durationis in 100ns ticks, whileself.start_time.0is raw QPC counts. Subtracting them directly and then dividing byperf_freqmixes units and can skew timestamps (and A/V sync). Convert both to seconds (or both to 100ns) before differencing.Apply this minimal, localized diff:
- let elapsed = frame.timestamp().Duration - self.start_time.0; - let display_time = self - .start_time - .1 - .checked_add(Duration::from_secs_f64( - elapsed as f64 / self.perf_freq as f64, - )) - .unwrap(); + // Convert both timelines to seconds before differencing: + let frame_secs = frame.timestamp().Duration as f64 / 10_000_000.0; + let start_secs = self.start_time.0 as f64 / self.perf_freq as f64; + let elapsed_secs = (frame_secs - start_secs).max(0.0); + let display_time = self + .start_time + .1 + .checked_add(Duration::from_secs_f64(elapsed_secs)) + .unwrap();Reference:
Direct3D11CaptureFrame.SystemRelativeTimereports QPC time represented as a TimeSpan (100ns units). (github.com)
🧹 Nitpick comments (1)
src/capturer/engine/win/mod.rs (1)
79-86: Optional: simplify by anchoring to the first frame’s timestamp (avoid QPC entirely).Store the first frame’s
timestamp().Durationand compute deltas purely in 100ns, then add tostart_time.1. This removes QPC math and frequency assumptions.Example (outside the shown hunk):
// in Capturer: start_ts_100ns: Option<i64>, // in on_frame_arrived: let base = *self.start_ts_100ns.get_or_insert(frame.timestamp().Duration); let elapsed_100ns = (frame.timestamp().Duration - base).max(0); let display_time = self.start_time.1.checked_add( Duration::from_secs_f64(elapsed_100ns as f64 / 10_000_000.0) ).unwrap();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/capturer/engine/win/mod.rs(1 hunks)
🔇 Additional comments (2)
src/capturer/engine/win/mod.rs (2)
79-79: Good catch: switch totimestamp()matches the current windows-capture API.The crate exposes
Frame::timestamp() -> TimeSpanin recent versions; older versions hadtimespan(). This change unblocks Windows builds against newer windows-capture. (docs.rs)
79-79: API rename sanity check passed No.timespan()calls remain—only.timestamp()is in use.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib.rs (1)
9-9: Gate or document the new re-export to avoid unconditional panic on Linux.Re-exporting
get_target_dimensionsmakes it part of the public API. On Linux, the current implementation intargets::modisunreachable!(), so exposing it at the crate root can lead to guaranteed panics at runtime on that platform. Consider gating the re-export to macOS/Windows (or documenting the Linux limitation clearly). Also, addingdoc(cfg)helps docs reflect platform availability.Proposed diff:
-// Helper Methods -pub use targets::{get_all_targets, get_main_display, get_target_dimensions}; +// Helper Methods +pub use targets::{get_all_targets, get_main_display}; +#[cfg(any(target_os = "macos", target_os = "windows"))] +#[cfg_attr(docsrs, doc(cfg(any(target_os = "macos", target_os = "windows"))))] +pub use targets::get_target_dimensions;Follow-ups:
- If you intend to keep it cross-platform in the root API, consider returning a Result on Linux instead of
unreachable!().- Adding a new public item is a semver-minor change; ensure the next release version reflects this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/lib.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib.rs (3)
src/targets/mac/mod.rs (1)
get_target_dimensions(103-117)src/targets/mod.rs (1)
get_target_dimensions(77-86)src/targets/win/mod.rs (1)
get_target_dimensions(83-105)
|
can someone merge this ? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/capturer/engine/linux/mod.rs (1)
8-8: Timestamp semantics changed toSystemTime::now(); consider cleaning up unused PTS and double‑check cross‑platform expectationsUsing
SystemTime::now()fordisplay_timeand wrapping payloads asFrame::Video(VideoFrame::RGBx/RGB/XBGR/BGRx)looks consistent with the newVideoFrameAPI and compiles cleanly for Linux.Two follow‑ups seem worthwhile:
- Unused PipeWire PTS:
get_timestamp(buffer)is still called (assigned totimestamp) but no longer used when constructingVideoFrame. This means we pay the cost of the unsafe meta walk every frame without benefit. If you don’t plan to use the PipeWire PTS any more, consider either:
Removing the call entirely, e.g.:
let timestamp = unsafe { get_timestamp(buffer) };let system_time = SystemTime::now();
let system_time = SystemTime::now();- And, if not used anywhere else, deleting `get_timestamp` as well. Or, if you want to keep it around for future work, at least acknowledge it as intentionally unused to avoid lint noise: ```diff
let timestamp = unsafe { get_timestamp(buffer) };
let _timestamp = unsafe { get_timestamp(buffer) };
- Meaning of
display_time: Previously this likely represented the stream/capture timestamp derived from PipeWire’s PTS; now it’s wall‑clockSystemTime. If any downstream code assumesdisplay_timeis tied to the capture clock (e.g., for sync with audio or to align Linux/Windows behavior), it may be worth:
- Confirming Windows and Linux backends now use the same notion of time, and
- Adding a brief comment on
VideoFrame::display_timedocumenting that it is wall‑clock time at delivery, not the underlying PipeWire PTS.These are non‑blocking but will reduce confusion around the Windows timestamp fix and keep the Linux side tidy.
Also applies to: 34-35, 122-161
2 participants
Am not able to compile scap on windows, because of this typo
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.