Repository navigation
quest(m1): plan exact timestamps in every binding - #5890
Conversation
Bindings carry {value, scale} timestamps mirroring moq_net::Timestamp
instead of lossy microsecond fields, after #5677 lands.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 25 minutes. View limit details
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 069d4bd7ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **Shape mirrors the core:** one `{value, scale}` per value, `value` at most | ||
| `2^62 - 1` and `scale` non-zero. The FFI boundary refuses anything else with |
There was a problem hiding this comment.
When a caller supplies a scale above 2^62 - 1, this contract treats it as valid because it only requires a nonzero scale, but moq_net::Timescale::new rejects such values. That makes the proposed record fail to mirror the core and may leave wrappers accepting timestamps that fail only when passed across the FFI boundary. Specify the same upper bound for scale and include an out-of-range-scale verification case.
AGENTS.md reference: AGENTS.md:L57-L64
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed: Timescale::new caps the scale at 2^62 - 1 too. Fixed in c783689: the shape now bounds scale like the core, and verification adds an out-of-range scale case.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merge summaryAdds the m1 quest Exact timestamps in every binding [L] and ranks it in Decisions are the eight settled with the maintainer in the PR body (bindings only, m1, after #5677, Review: Codex on 069d4bd found one issue (bound (Written by Claude Opus 5.5) |
Summary
Adds
quest/m1/binding-timestamps.md[L]: every binding (moq-ffi, moq-c, cpp/obs, py/swift/kt/go/dart) carries media timestamps as an exact{value, scale}record mirroringmoq_net::Timestamp, replacing the lossy microsecond fields. Ranked in m1 right after Data consumer timestamps (#5677), which lands first with the interim microsecond fields.Quest-only. Public API and wire impact: none in this PR (the planned work breaks every binding's API, no wire change).
Decisions (planned with the maintainer, 2026-10-18)
{value, scale}timestamps with untimed as absent, replacing microseconds on frames, datagrams, producer args and data values; core and JS untouched.MoqTimestamp/ Cmoq_timestamp/ wrappersTimestampMediaTimeconst moq_timestamp *scale == 0sentinelhas_timestampflagConsidered and rejected without a prompt: raw ticks at the track scale beside
MoqTrackInfo.timescale(a producer argument needs its own scale; per-value records match the core and JS).Checks
quest check: 410 documents ok.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code