Conversation
There was a problem hiding this comment.
Pull request overview
This PR converts the repository from a template crate into mediatime, implementing exact rational media-time primitives (Timebase, Timestamp, TimeRange) with no_std-first APIs, plus accompanying docs, tests, benchmarks, and CI/config updates.
Changes:
- Implements
Timebase,Timestamp, andTimeRange(with hashing/ordering semantics) and adds an extensive in-crate test suite. - Updates crate metadata and README to reflect the new crate name, API, and MSRV.
- Adds a Criterion benchmark and Codecov configuration; tweaks formatting/CI workflow settings.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib.rs |
Adds the full public API for timebase/timestamp/range types plus tests. |
Cargo.toml |
Renames crate to mediatime, sets MSRV/edition/metadata, bench config. |
README.md |
Rewrites documentation to describe the new crate and usage examples. |
CHANGELOG.md |
Adds an initial changelog for 0.1.0. |
benches/gcd.rs |
Adds Criterion benchmarks for GCD helpers used in hashing. |
.codecov.yml |
Adds Codecov configuration. |
.github/workflows/ci.yml |
Updates CI configuration (including schedule formatting and tool versions). |
.github/workflows/loc.yml |
Updates LoC gist upload key/name. |
rustfmt.toml |
Adjusts rustfmt configuration (imports granularity). |
tests/foo.rs |
Removes the placeholder test file. |
README-zh_CN.md |
Removes the Chinese template README. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if diff < 0 { | ||
| return None; | ||
| } | ||
| let secs = (diff / NS_PER_SEC) as u64; |
There was a problem hiding this comment.
duration_since computes secs/nanos from an i128 diff, but then casts to u64/u32 without any overflow handling. For large pts/timebases this will wrap (not saturate), which contradicts the doc comment about pathological inputs saturating. Consider clamping to Duration::MAX (or returning None/an error) when diff / NS_PER_SEC exceeds u64::MAX.
| let secs = (diff / NS_PER_SEC) as u64; | |
| let secs_i128 = diff / NS_PER_SEC; | |
| if secs_i128 > u64::MAX as i128 { | |
| return Some(Duration::MAX); | |
| } | |
| let secs = secs_i128 as u64; |
| // = pts * from.num * to.den / (from.den * to.num) | ||
| let numerator = (pts as i128) * (from.num as i128) * (to.den.get() as i128); | ||
| let denominator = (from.den.get() as i128) * (to.num as i128); | ||
| (numerator / denominator) as i64 |
There was a problem hiding this comment.
rescale_pts divides using an i128 intermediate but then casts the quotient to i64. If the rescaled value exceeds the i64 range, this will wrap/truncate silently via as i64. Consider defining and enforcing overflow behavior (e.g., saturate to i64::{MIN,MAX} or return an Option/Result) to avoid surprising incorrect timestamps on large inputs.
| - **benches/* | ||
| - **examples/* | ||
| - **tests/* |
There was a problem hiding this comment.
These ignore entries start with * (e.g. **benches/*), which YAML parsers commonly interpret as an alias token; unquoted, this is likely invalid YAML. Quote the glob patterns (and consider using **/benches/* etc.) so .codecov.yml parses reliably.
| - **benches/* | |
| - **examples/* | |
| - **tests/* | |
| - "**benches/*" | |
| - "**examples/*" | |
| - "**tests/*" |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let numerator = (pts as i128) * (from.num as i128) * (to.den.get() as i128); | ||
| let denominator = (from.den.get() as i128) * (to.num as i128); | ||
| let q = numerator / denominator; |
There was a problem hiding this comment.
rescale_pts documents that it panics when to.num() == 0, but the implementation relies on an implicit division-by-zero panic (numerator / denominator). Consider adding an explicit assert!(to.num != 0, ...) (or similar) before computing q so callers get a clear panic message and the behavior is intentional/documented in-code.
| /// Works across different timebases. Computes the difference in nanoseconds | ||
| /// via 128-bit intermediates; for realistic video PTS ranges this is exact. | ||
| /// If the result would exceed `Duration::MAX` (pathological: seconds don't | ||
| /// fit in `u64`), saturates to `Duration::MAX` rather than wrapping. | ||
| #[cfg_attr(not(tarpaulin), inline(always))] | ||
| pub const fn duration_since(&self, earlier: &Self) -> Option<Duration> { | ||
| // nanos = pts * tb.num * 1_000_000_000 / tb.den | ||
| const NS_PER_SEC: i128 = 1_000_000_000; | ||
| let self_ns = (self.pts as i128) * (self.timebase.num as i128) * NS_PER_SEC | ||
| / (self.timebase.den.get() as i128); | ||
| let earlier_ns = (earlier.pts as i128) * (earlier.timebase.num as i128) * NS_PER_SEC | ||
| / (earlier.timebase.den.get() as i128); | ||
| let diff = self_ns - earlier_ns; | ||
| if diff < 0 { | ||
| return None; | ||
| } | ||
| let secs_i128 = diff / NS_PER_SEC; | ||
| if secs_i128 > u64::MAX as i128 { | ||
| return Some(Duration::MAX); | ||
| } | ||
| let secs = secs_i128 as u64; | ||
| let nanos = (diff % NS_PER_SEC) as u32; | ||
| Some(Duration::new(secs, nanos)) |
There was a problem hiding this comment.
duration_since converts each timestamp to nanoseconds via integer division and then subtracts. For mixed timebases this can differ (by up to 1 ns) from rounding the exact rational difference once (e.g., 1 tick @ 1/3 minus 1 tick @ 1/6). If the intent is “round exact difference toward zero”, compute the diff using a single cross-multiply to a common denominator and do one final division/rounding; otherwise update the doc comment to reflect per-endpoint truncation.
| /// Works across different timebases. Computes the difference in nanoseconds | |
| /// via 128-bit intermediates; for realistic video PTS ranges this is exact. | |
| /// If the result would exceed `Duration::MAX` (pathological: seconds don't | |
| /// fit in `u64`), saturates to `Duration::MAX` rather than wrapping. | |
| #[cfg_attr(not(tarpaulin), inline(always))] | |
| pub const fn duration_since(&self, earlier: &Self) -> Option<Duration> { | |
| // nanos = pts * tb.num * 1_000_000_000 / tb.den | |
| const NS_PER_SEC: i128 = 1_000_000_000; | |
| let self_ns = (self.pts as i128) * (self.timebase.num as i128) * NS_PER_SEC | |
| / (self.timebase.den.get() as i128); | |
| let earlier_ns = (earlier.pts as i128) * (earlier.timebase.num as i128) * NS_PER_SEC | |
| / (earlier.timebase.den.get() as i128); | |
| let diff = self_ns - earlier_ns; | |
| if diff < 0 { | |
| return None; | |
| } | |
| let secs_i128 = diff / NS_PER_SEC; | |
| if secs_i128 > u64::MAX as i128 { | |
| return Some(Duration::MAX); | |
| } | |
| let secs = secs_i128 as u64; | |
| let nanos = (diff % NS_PER_SEC) as u32; | |
| Some(Duration::new(secs, nanos)) | |
| /// Works across different timebases. Computes the exact rational difference | |
| /// first using a common denominator, then truncates once when converting to | |
| /// nanoseconds for the returned [`Duration`]. | |
| /// If the result would exceed `Duration::MAX` (pathological: seconds don't | |
| /// fit in `u64`), saturates to `Duration::MAX` rather than wrapping. | |
| #[cfg_attr(not(tarpaulin), inline(always))] | |
| pub const fn duration_since(&self, earlier: &Self) -> Option<Duration> { | |
| const NS_PER_SEC: i128 = 1_000_000_000; | |
| let self_den = self.timebase.den.get(); | |
| let earlier_den = earlier.timebase.den.get(); | |
| let mut a = self_den; | |
| let mut b = earlier_den; | |
| while b != 0 { | |
| let r = a % b; | |
| a = b; | |
| b = r; | |
| } | |
| let gcd = a as i128; | |
| let self_scale = (earlier_den as i128) / gcd; | |
| let earlier_scale = (self_den as i128) / gcd; | |
| let common_den = (self_den as i128) * self_scale; | |
| let diff_num = (self.pts as i128) * (self.timebase.num as i128) * self_scale | |
| - (earlier.pts as i128) * (earlier.timebase.num as i128) * earlier_scale; | |
| if diff_num < 0 { | |
| return None; | |
| } | |
| let secs_i128 = diff_num / common_den; | |
| if secs_i128 > u64::MAX as i128 { | |
| return Some(Duration::MAX); | |
| } | |
| let rem = diff_num % common_den; | |
| let nanos = (rem * NS_PER_SEC / common_den) as u32; | |
| Some(Duration::new(secs_i128 as u64, nanos)) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
.github/workflows/loc.yml:50
- The workflow reads
tokeit.json, but the preceding step runstokeit --lang rustwithout writing JSON output to that file. As written, theUpload total loc to GitHub Giststep will fail with a missing file; emit JSON (e.g.,tokeit --output json > tokeit.json) or adjust the script to read from stdout/artifacts.
- name: Count total lines of code
run: |
tokeit --lang rust
- name: Upload total loc to GitHub Gist
uses: actions/github-script@v9
with:
github-token: ${{ secrets.GIST_PAT }}
script: |
const fs = require('fs');
const output = fs.readFileSync('tokeit.json', 'utf8');
const gistId = '327b2a8aef9003246e45c6e47fe63937';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| accessors, `with_*`/`set_*` setters for both endpoints, `is_instant`. | ||
| - `const fn` across the whole public surface — every constructor, accessor, | ||
| and setter can be evaluated in a `const` context. | ||
| - `#![no_std]` always, zero dependencies. No allocation anywhere — every |
There was a problem hiding this comment.
Changelog claims "#![no_std] always", but the crate uses #![cfg_attr(not(test), no_std)] (tests/doctests build with std). Consider rewording to something like "no_std by default" or "no_std in non-test builds" to match the actual crate configuration.
| - `#![no_std]` always, zero dependencies. No allocation anywhere — every | |
| - `#![no_std]` in non-test builds, zero dependencies. No allocation anywhere — every |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.