Fix windows compilation because of timestamp typo #176
Conversation
WalkthroughReplaced frame.timespan().Duration with frame.timestamp().Duration in on_frame_arrived (Windows capturer). Added a public re-export of get_target_dimensions in src/lib.rs. No other control flow, logic, or public signatures were modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
Am not able to compile scap on windows, because of this typo
Summary by CodeRabbit
New Features
Bug Fixes