Add AbsoluteCaptureTime extension and related functionality#864
Add AbsoluteCaptureTime extension and related functionality#864algesten merged 5 commits intoalgesten:mainfrom
Conversation
|
@lmiguelgato also add an integration test (in the test folder) |
algesten
left a comment
There was a problem hiding this comment.
Overall
Good, well-structured addition that follows the existing extension patterns closely. The serialization/deserialization logic is correct and the tests cover the key format variations. A few items to address:
Issues
-
requires_two_byte_formis unnecessarily conservative — Per RFC 8285, one-byte form supports payloads up to 16 bytes. The 16-byte extended format fits, so forcing two-byte form whenabs_capture_clock_offsetis present wastes header space. The self-contradicting comment confirms the confusion. Either remove the override or document a specific interop reason. -
Remove leftover
eprintln!insrc/lib.rs— debug print inevent_is_reasonably_sizedtest. -
abs_capture_clock_offsetis an opaquei64with no documented format — The spec defines this as a signed NTP fixed-point number (SQ32.32). Without documentation, callers will misinterpret the unit. At minimum add a doc comment; ideally provide a conversion helper. -
Potential
Instantunderflow in parse path — If an incoming NTP timestamp maps to a time beforeBEGINNING_OF_TIME, the subtractionalready_happened() + duration_since_epoch - epoch_to_beginning()can panic. Consider achecked_sub/saturating approach for robustness against malformed packets.
Nits
- Byte parsing can use
buf[..8].try_into().unwrap()instead of listing all 8 indices.
Observations (non-blocking)
- The extension is not added to
ExtensionMap::standard(), meaning it won't be negotiated unless the application explicitly configures it. This seems intentional and fine for an experimental extension, just worth calling out. - The event size bump from
< 470to< 490gives ~18 bytes of headroom, which is reasonable.
Fundamental type issue:
|
|
To clarify the // Field (sr.rs:29)
pub ntp_time: SystemTime,
// Write (sr.rs:75-76)
let mt = self.ntp_time.as_ntp_64();
buf[4..12].copy_from_slice(&mt.to_be_bytes());
// Parse (sr.rs:123-126)
let ntp_time = u64::from_be_bytes([buf[4], buf[5], buf[6], buf[7], buf[8], buf[9], buf[10], buf[11]]);
let ntp_time = SystemTime::from_ntp_64(ntp_time);The |
|
If you want to discuss stuff, you can reach me on Discord also. |
Added test case Also added test case |
This makes total sense. Thanks for bringing it up. I have changed it to |
193435b to
c31c374
Compare
|
Rebased and squashed my changes after addressing the code review comments. Updated PR description. |
|
change lgtm |
|
@algesten are you good with the changes? |
algesten
left a comment
There was a problem hiding this comment.
Made a tweak AbsCaptureTime to encapsulate these two values since they don't exist independently.
|
@algesten changelog entry? |
Group the two separate fields (abs_capture_time, abs_capture_clock_offset) into a single AbsCaptureTime struct with public fields, eliminating the invalid state where clock_offset is set without a capture_time. Add clock_offset_secs()/set_clock_offset() helper methods to convert between the raw NTP Q32.32 wire format and f64 seconds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes unused import lint error in CI (-D warnings). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
71ffe85 to
b315142
Compare
|
Thanks! |
Add support for abs-capture-time RTP header extension
Summary
Implements the
abs-capture-timeRTP header extension as specified in the WebRTC specification. This extension enables accurate audio/video synchronization in scenarios where media passes through RTCP-terminating mixers or other intermediary systems.Motivation
The
abs-capture-timeextension addresses a critical limitation in multi-hop WebRTC scenarios. When RTP packets traverse mixers or SFUs that terminate RTCP (Sender Reports), receivers lose the ability to correlate timestamps from the original capture system, breaking A/V sync. This extension embeds the original capture timestamp in each RTP packet, preserving synchronization information end-to-end.Key use cases:
Implementation Details
Extension Format
http://www.webrtc.org/experiments/rtp-hdrext/abs-capture-timeChanges
New Extension Variant (
src/rtp/ext.rs:27)Extension::AbsoluteCaptureTimeenum variantExtension Values (
src/rtp/ext.rs:866-869)abs_capture_time: Option<SystemTime>- Absolute NTP timestamp of original frame captureabs_capture_clock_offset: Option<i64>- Optional sender's clock offset estimate (raw NTP Q32.32 format)Serialization/Parsing (
src/rtp/ext.rs)SystemTimeand 64-bit NTP format (UQ32.32) usingas_ntp_64()/from_ntp_64()SenderInfoinsrc/rtp/rtcp/sr.rsMedia Type Support
abs-send-timeextensionDesign Decision: SystemTime vs Instant
This implementation uses
SystemTimeforabs_capture_timerather thanInstant:Why SystemTime: The
abs-capture-timeextension represents an absolute NTP wall-clock timestamp that must be preserved across machines and network hops.Instantis a monotonic clock relative to an arbitrary local epoch and cannot represent absolute timestamps from remote systems.Precedent: This follows the exact pattern of
SenderInfo.ntp_timeinsrc/rtp/rtcp/sr.rs, which also usesSystemTimefor NTP timestamps.Simplification: Using
SystemTimeenables direct NTP conversions (as_ntp_64()/from_ntp_64()) without complex epoch calculations.Note: This differs from
abs-send-time, which usesInstantbecause it's a 24-bit truncated value used only for relative timing/bandwidth estimation, not as an absolute timestamp.Testing
abs_capture_time_short_form- 8-byte format (timestamp only)abs_capture_time_extended_form- 16-byte format with clock offsetabs_capture_time_two_byte_form- Two-byte header form (extension ID > 14)tests/abs-capture-time.rs):abs_capture_time_negotiation- Verifies SDP negotiation and packet deliveryabs_capture_time_sdp_roundtrip- Verifies extension URI in SDPEvent Size Impact
The addition of two
Optionfields toExtensionValuesincreased theEventenum size by 2 bytes (470 → 472 bytes). Updated the size limit to 490 bytes to provide reasonable headroom for future additions.Usage Example
Bandwidth Considerations
Per the specification, implementations should not send this extension with every packet to save bandwidth. Senders should include it:
This implementation provides the mechanism; rate limiting policy is left to the application layer.
Compatibility
References
abs-send-time(already implemented in str0m)