feat(interceptor): add GCC sender-side bandwidth estimator#85
feat(interceptor): add GCC sender-side bandwidth estimator#85nightness wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds sender-side GCC bandwidth estimation (TWCC-driven) and a receiver-side jitter buffer interceptor, exporting both via the rtc-interceptor crate API.
Changes:
- Introduces
GccInterceptor(trendline overuse detection + AIMD rate controller) with a pollingGccHandle. - Adds
JitterBufferInterceptorwith adaptive playout delay (RFC 3550 jitter-based) and optional playout-delay RTP extension support. - Re-exports new interceptors/builders and adds the
bytesdependency for TWCC header extension parsing.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| rtc-interceptor/src/lib.rs | Registers new gcc and jitter_buffer modules and re-exports public builders/handles. |
| rtc-interceptor/src/jitter_buffer/mod.rs | Adds jitter buffer interceptor + builder, integrates with stream binding and scheduling. |
| rtc-interceptor/src/jitter_buffer/stream.rs | Implements per-SSRC jitter buffer logic, delay adaptation, and unit tests. |
| rtc-interceptor/src/gcc/mod.rs | Adds GCC interceptor, handle API, TWCC feedback parsing, and end-to-end tests. |
| rtc-interceptor/src/gcc/rate_controller.rs | Implements AIMD controller and tests. |
| rtc-interceptor/src/gcc/trendline.rs | Implements trendline filter + threshold adaptation and tests. |
| rtc-interceptor/Cargo.toml | Adds bytes workspace dependency. |
Comments suppressed due to low confidence (1)
rtc-interceptor/src/gcc/mod.rs:1
with_min_bitrate/with_max_bitrateallow creating an invalid configuration wheremin_bitrate_bps > max_bitrate_bps. With the currentAimdRateController, this can produce estimates that exceed the intended max (e.g., decreases clamp with.max(min_bps)even ifmin_bps > max_bps). Please enforcemin <= maxat builder time (e.g., clamp, swap, or return aResultfrombuild()/new()variants) so the public API cannot create a contradictory controller configuration.
//! GCC (Google Congestion Control) sender-side bandwidth estimator.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Packet::Rtcp(ref rtcp_pkts) = msg.message { | ||
| let now = msg.now; | ||
| // Collect feedback packets first to avoid borrow issues. | ||
| let feedbacks: Vec<TransportLayerCc> = rtcp_pkts | ||
| .iter() | ||
| .filter_map(|pkt| { | ||
| pkt.as_any() | ||
| .downcast_ref::<TransportLayerCc>() | ||
| .cloned() | ||
| }) | ||
| .collect(); | ||
| for fb in &feedbacks { | ||
| self.process_feedback(fb, now); | ||
| } | ||
| } |
There was a problem hiding this comment.
This clones every TransportLayerCc in the RTCP compound packet just to process them. TWCC feedback packets can be relatively large, and cloning them per read adds avoidable allocation/copy overhead. You should be able to iterate the RTCP packets by reference and call process_feedback on &TransportLayerCc directly without collecting/cloning first (process before forwarding to inner.handle_read(msg)).
| if let Packet::Rtcp(ref rtcp_pkts) = msg.message { | |
| let now = msg.now; | |
| // Collect feedback packets first to avoid borrow issues. | |
| let feedbacks: Vec<TransportLayerCc> = rtcp_pkts | |
| .iter() | |
| .filter_map(|pkt| { | |
| pkt.as_any() | |
| .downcast_ref::<TransportLayerCc>() | |
| .cloned() | |
| }) | |
| .collect(); | |
| for fb in &feedbacks { | |
| self.process_feedback(fb, now); | |
| } | |
| } | |
| let now = msg.now; | |
| let feedbacks: Vec<&TransportLayerCc> = { | |
| if let Packet::Rtcp(ref rtcp_pkts) = msg.message { | |
| rtcp_pkts | |
| .iter() | |
| .filter_map(|pkt| pkt.as_any().downcast_ref::<TransportLayerCc>()) | |
| .collect() | |
| } else { | |
| Vec::new() | |
| } | |
| }; | |
| for fb in feedbacks { | |
| self.process_feedback(fb, now); | |
| } |
There was a problem hiding this comment.
Addressed: eliminated the intermediate Vec<&TransportLayerCc> collection. Now iterates rtcp_pkts inline with filter_map + downcast_ref directly. Should be marked outdated.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #85 +/- ##
==========================================
+ Coverage 71.17% 71.33% +0.16%
==========================================
Files 442 447 +5
Lines 67330 67794 +464
==========================================
+ Hits 47922 48361 +439
- Misses 19408 19433 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Fix all clippy collapsible_if warnings (8 instances across gcc/mod.rs, jitter_buffer/mod.rs, and jitter_buffer/stream.rs) - Remove unused packet_size from send_times HashMap (store only send_time_ms) - Avoid cloning TransportLayerCc packets; iterate by reference instead - Eliminate per-chunk Vec allocation in expand_chunk by inlining iteration - Clamp initial_delay to [min_delay, max_delay] in JitterBufferStream::new() - Fix permanently tightened bounds: use per-packet effective min/max instead of mutating the configured stream-wide min_delay/max_delay Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.inner | ||
| .lock() | ||
| .map(|g| g.target_bitrate_bps) | ||
| .unwrap_or(None) | ||
| } |
There was a problem hiding this comment.
target_bitrate_bps() silently returns None if the mutex is poisoned, which can hide bugs and cause the app to stop adapting bitrate without any signal. Consider recovering via unwrap_or_else(|e| e.into_inner()) (and/or logging) so callers still get the last known estimate.
There was a problem hiding this comment.
Addressed: target_bitrate_bps() now recovers from poison via unwrap_or_else(|e| e.into_inner()) with a log::warn, so callers always get the last known estimate. Should be marked outdated.
- Remove unnecessary Bytes::copy_from_slice; clone Bytes (refcount bump) - Eliminate temporary Vec<&TransportLayerCc> allocation; iterate inline - Recover from mutex poison in target_bitrate_bps() and process_feedback() via unwrap_or_else(|e| e.into_inner()) with log::warn - Fix playout-delay ordering: compute jitter-based release first, then clamp to per-packet sender bounds (avoids violating effective min/max) - Remove unused bytes::Bytes import - Add tests: initial_delay clamping, parse_playout_delay, playout-delay applied after compute_release, loss_fraction with StatusVectorChunk and empty chunks, builder custom bitrate bounds, GccHandle clone shares state - Apply cargo fmt to rate_controller.rs and trendline.rs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a new `JitterBufferInterceptor` that buffers incoming RTP packets per SSRC and releases them in sequence order after an adaptive playout delay computed from the RFC 3550 §A.8 jitter formula. - `JitterBufferStream` (stream.rs): per-SSRC packet buffer with adaptive target delay; force-release after max_delay to prevent starvation; playout-delay RTP extension (ietf WebRTC draft) for sender-side hints - `JitterBufferInterceptor<P>` (mod.rs): wraps inner chain; buffers RTP for tracked SSRCs, passes RTCP and untracked-SSRC packets immediately; drains ready packets into inner chain in handle_timeout + poll_read - `JitterBufferBuilder`: configurable min/max/initial delay with sensible defaults (20 ms / 500 ms / 50 ms) - Jitter update skips out-of-order RTP timestamps to prevent spurious delay spikes from reordered packets - 15 unit tests across stream.rs and mod.rs; all 129 interceptor tests pass Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix all clippy collapsible_if warnings (stream.rs and mod.rs) - Reject duplicate sequence numbers already in the buffer - Make playout-delay extension per-packet instead of permanently mutating min/max bounds - Guard against non-monotonic time with checked_duration_since - Guard against zero clock_rate to prevent division by zero / NaN panic - Replace Instant::now() in poll_read with tracked last_now from handle_read/handle_timeout - Fix test_jitter_adapts_target_delay to use strictly increasing arrival times - Fix misleading comment in test_unbound_ssrc_passes_through and add assertion Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ates Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cherry-picked from 9371e86 and rebased onto feat/jitter-buffer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix all clippy collapsible_if warnings (8 instances across gcc/mod.rs, jitter_buffer/mod.rs, and jitter_buffer/stream.rs) - Remove unused packet_size from send_times HashMap (store only send_time_ms) - Avoid cloning TransportLayerCc packets; iterate by reference instead - Eliminate per-chunk Vec allocation in expand_chunk by inlining iteration - Clamp initial_delay to [min_delay, max_delay] in JitterBufferStream::new() - Fix permanently tightened bounds: use per-packet effective min/max instead of mutating the configured stream-wide min_delay/max_delay Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unnecessary Bytes::copy_from_slice; clone Bytes (refcount bump) - Eliminate temporary Vec<&TransportLayerCc> allocation; iterate inline - Recover from mutex poison in target_bitrate_bps() and process_feedback() via unwrap_or_else(|e| e.into_inner()) with log::warn - Fix playout-delay ordering: compute jitter-based release first, then clamp to per-packet sender bounds (avoids violating effective min/max) - Remove unused bytes::Bytes import - Add tests: initial_delay clamping, parse_playout_delay, playout-delay applied after compute_release, loss_fraction with StatusVectorChunk and empty chunks, builder custom bitrate bounds, GccHandle clone shares state - Apply cargo fmt to rate_controller.rs and trendline.rs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ed320ac to
d1b6240
Compare
Summary
Add
GccInterceptor— a Google Congestion Control (GCC, RFC 8698) bandwidth estimator that uses TWCC feedback to dynamically estimate available send bandwidth.Key components:
TrendlineFilter— sliding window (N=20) OLS linear regression on inter-packet delay gradients; dynamic overuse threshold [6ms, 600ms]AimdRateController— AIMD state machine: 1.08×/sec increase on Normal/Underusing, ×0.85 on Overuse; loss-based adjustment (> 10% loss → decrease)GccHandle—Arc<Mutex<GccShared>>output channel (no changes to framework'sEout = ()contract)GccInterceptorBuilder::new()returns(builder, GccHandle)pairChain position (GCC must be inner to TwccSender):
Usage:
Also includes the JitterBuffer interceptor (receiver-side, RFC 3550 §A.8 adaptive playout delay) as a dependency.
Review feedback addressed
collapsible_ifwarnings (8 instances)packet_sizefromsend_timesHashMapTransportLayerCcpackets; iterate by referenceVecallocation by inlining iterationinitial_delayto[min_delay, max_delay]inJitterBufferStream::new()Bytes::copy_from_slice; cloneBytes(cheap refcount bump)Vec<&TransportLayerCc>allocation; iterate inlinetarget_bitrate_bps()andprocess_feedback()viaunwrap_or_elsewithlog::warnTest plan
cargo test -p rtc-interceptor— all 154 tests pass (7 new tests added)cargo clippy -p rtc-interceptor— zero warningscargo fmt --check— clean🤖 Generated with Claude Code