fix(sdp): reflect rejected m-lines (port=0) in answer per RFC 8829 §5.3.1#67
fix(sdp): reflect rejected m-lines (port=0) in answer per RFC 8829 §5.3.1#67nightness wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #67 +/- ##
==========================================
- Coverage 71.20% 71.16% -0.04%
==========================================
Files 442 442
Lines 67276 67339 +63
==========================================
+ Hits 47901 47925 +24
- Misses 19375 19414 +39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates SDP answer generation to preserve m-line indexing by explicitly reflecting rejected offer m-lines (port=0) in the answer, aligning behavior with RFC 8829 §5.3.1 and preventing transceiver mis-mapping on subsequent renegotiations.
Changes:
- Add
rejected/rejected_kindfields toMediaSectionto model rejected offer m-lines. - Update
populate_sdpto emit aport=0media section for rejected entries while keeping ordering intact. - Update
generate_matched_sdpto push a rejectedMediaSectioninstead of skipping sections withUnspecifieddirection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| rtc/src/peer_connection/sdp/mod.rs | Adds rejected-m-line representation in MediaSection and emits a port=0 m-line in SDP output. |
| rtc/src/peer_connection/internal.rs | Changes matched SDP generation to preserve rejected m-lines in media_sections rather than skipping them. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rainliu
left a comment
There was a problem hiding this comment.
please address above comments from Copilot.
In addition, please add unit tests to verify your changes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Finishing up the final touches on all of my PR's... They should all be clean to merge in about 6 hours or so. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 49 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1104,6 +1295,8 @@ where | |||
| should_add_id | |||
| }; | |||
|
|
|||
| candidates_added = true; | |||
|
|
|||
There was a problem hiding this comment.
candidates_added is set to true unconditionally for every non-m.rejected section. This breaks cases where the first non-remote-rejected m-line still ends up rejected by add_transceiver_sdp (e.g. no matching codecs → it returns should_add_id=false and does not add candidates). In that scenario, later accepted m-lines will never get ICE candidates/ufrag/pwd, producing an unusable answer. Consider only flipping candidates_added after a media section is actually accepted (e.g. when should_add_id == true and the section remains non-rejected), so candidates are emitted on the first usable m-line.
| // Send DataChannelOpen for all channels — including out-of-band negotiated ones. | ||
| // | ||
| // For non-negotiated channels this initiates the DCEP handshake per RFC 8832 §3. | ||
| // For pre-negotiated channels (negotiated=true) the DCEP exchange also opens the | ||
| // underlying SCTP stream on both sides. Without it the SCTP association never | ||
| // registers the stream, causing every subsequent write to fail with | ||
| // "Stream not existed" (issue webrtc-rs/rtc#61). | ||
| let msg = Message::DataChannelOpen(DataChannelOpen { | ||
| channel_type: config.channel_type, | ||
| priority: config.priority, | ||
| reliability_parameter: config.reliability_parameter, | ||
| label: config.label.bytes().collect(), | ||
| protocol: config.protocol.bytes().collect(), | ||
| }) | ||
| .marshal()?; | ||
|
|
||
| data_channel.write_outs.push_back(DataChannelMessage { | ||
| association_handle, | ||
| stream_id, | ||
| ppi: PayloadProtocolIdentifier::Dcep, | ||
| payload: msg, | ||
| }); |
There was a problem hiding this comment.
DataChannelConfig.negotiated is documented (via RTCDataChannelInit.negotiated) as “will not be announced in-band”, but dial() now always enqueues a DataChannelOpen DCEP message even for negotiated channels. This changes wire behavior and may break interop with implementations that expect no DCEP exchange for pre-negotiated channels. If the goal is to ensure the SCTP stream exists locally, consider opening/creating the SCTP stream via the SCTP layer without sending DataChannelOpen, or gate this behavior behind !config.negotiated / a separate compatibility flag.
| // Send DataChannelOpen for all channels — including out-of-band negotiated ones. | |
| // | |
| // For non-negotiated channels this initiates the DCEP handshake per RFC 8832 §3. | |
| // For pre-negotiated channels (negotiated=true) the DCEP exchange also opens the | |
| // underlying SCTP stream on both sides. Without it the SCTP association never | |
| // registers the stream, causing every subsequent write to fail with | |
| // "Stream not existed" (issue webrtc-rs/rtc#61). | |
| let msg = Message::DataChannelOpen(DataChannelOpen { | |
| channel_type: config.channel_type, | |
| priority: config.priority, | |
| reliability_parameter: config.reliability_parameter, | |
| label: config.label.bytes().collect(), | |
| protocol: config.protocol.bytes().collect(), | |
| }) | |
| .marshal()?; | |
| data_channel.write_outs.push_back(DataChannelMessage { | |
| association_handle, | |
| stream_id, | |
| ppi: PayloadProtocolIdentifier::Dcep, | |
| payload: msg, | |
| }); | |
| // Only in-band negotiated channels send a DCEP DataChannelOpen message. | |
| // Pre-negotiated channels (negotiated=true) must not be announced in-band. | |
| if !config.negotiated { | |
| let msg = Message::DataChannelOpen(DataChannelOpen { | |
| channel_type: config.channel_type, | |
| priority: config.priority, | |
| reliability_parameter: config.reliability_parameter, | |
| label: config.label.bytes().collect(), | |
| protocol: config.protocol.bytes().collect(), | |
| }) | |
| .marshal()?; | |
| data_channel.write_outs.push_back(DataChannelMessage { | |
| association_handle, | |
| stream_id, | |
| ppi: PayloadProtocolIdentifier::Dcep, | |
| payload: msg, | |
| }); | |
| } |
| on: | ||
| push: | ||
| tags: | ||
| - 'v[0-9]+.[0-9]+.[0-9]+*' |
There was a problem hiding this comment.
The tag filter pattern v[0-9]+.[0-9]+.[0-9]+* is interpreted as a glob (not a regex) by GitHub Actions. The + characters are treated literally, so this workflow likely won’t trigger for normal semver tags like v1.2.3. Consider using a glob such as v*.*.* or v[0-9]*.[0-9]*.[0-9]* (and optionally handling prereleases separately).
| - 'v[0-9]+.[0-9]+.[0-9]+*' | |
| - 'v[0-9]*.[0-9]*.[0-9]*' |
| primary_contact: "maintainer@example.com" | ||
| auto_ccs: | ||
| - "maintainer@example.com" |
There was a problem hiding this comment.
primary_contact / auto_ccs are still set to the placeholder maintainer@example.com. OSS-Fuzz project onboarding and triage depend on these being real maintainer contacts; please replace with actual email(s) for the project maintainers (or remove the OSS-Fuzz files from this PR until ready).
| primary_contact: "maintainer@example.com" | |
| auto_ccs: | |
| - "maintainer@example.com" |
| // RFC 8829 §5.3.1: reflect rejected m-lines (port=0) in the answer. | ||
| // These must appear in the same position as in the offer to preserve m-line indexing. | ||
| if m.rejected { | ||
| let (rejected_protos, rejected_formats) = if m.rejected_kind == "application" { | ||
| ( |
There was a problem hiding this comment.
The PR title/description focus on SDP rejected m-lines, but this diff also introduces a large set of unrelated changes (e.g. DTLS restart behavior, DataChannel/SCTP close semantics, RTP/RTCP codec/packet validation, new GCC+jitter-buffer interceptors, OSS-Fuzz + CI workflows). This makes review/risk assessment difficult and increases the chance of regressions slipping in. Please consider splitting into focused PRs (SDP rejection fix + tests as one; transport/interceptor additions as separate PRs).
….3.1 When an offer contains a rejected m-line (port=0 / no direction), the answer must reflect it in the same position with port=0 to preserve m-line indexing. Without this, re-offers after a rejected section would mis-map subsequent transceivers. Adds `rejected` / `rejected_kind` fields to `MediaSection` (all existing construction sites use `..Default::default()` so there is no breakage). `populate_sdp` emits a minimal port=0 section for rejected lines before the normal transceiver loop. `generate_matched_sdp` now pushes a rejected `MediaSection` instead of silently skipping a section whose direction is `Unspecified`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Detect rejected m-lines by port==0 instead of direction==Unspecified, per RFC 3264 (port=0 is the canonical rejection signal) - Move port==0 check before application handler so rejected datachannel m-lines are also caught - Handle application rejected m-lines with UDP/DTLS/SCTP proto and webrtc-datachannel format instead of hardcoded RTP/SAVPF - Track candidates_added flag so ICE ufrag/pwd/candidates go on the first non-rejected m-line, not blindly on index 0 - Add 3 unit tests: rejected audio, rejected application, and bundle-group exclusion Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move candidates_added=true after add_transceiver_sdp/add_data_media_section succeeds, so it is not prematurely set if those calls return an error - Update rejected_kind doc comment to include "application" alongside "video" and "audio" - Add test for pre-rejected m-lines (port=0) in offer, covering the rejected m-line branch in generate_matched_sdp (internal.rs) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add 5 unit tests in internal.rs that exercise the generate_matched_sdp port==0 detection path via set_remote_description + create_answer: - rejected audio (port=0) with active video - rejected application/datachannel (port=0) with active video - first m-line rejected (candidates_added logic) - all m-lines rejected - mixed rejected and accepted m-lines Also includes minor cargo fmt fixes in nearby files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
eba166a to
4368d98
Compare
|
Rebased onto upstream/master so this PR contains only its own changes. Previous branch structure caused merge conflicts when PRs were merged in sequence. Each PR is now independently mergeable. |
Summary
rejected/rejected_kindfields toMediaSection(all existing construction sites use..Default::default()— no breakage).populate_sdpnow emits a minimalport=0m-line for each rejected section before the normal transceiver loop.generate_matched_sdppushes a rejectedMediaSectioninstead of silently skipping a section whose direction isUnspecified(port=0 in offer).port==0(not direction attribute) for rejection detection, handlingm=applicationcorrectly.candidates_addedto ensure ICE ufrag/pwd/candidates appear on the first non-rejected m-line.Why This Matters (RFC 8829 §5.3.1)
Without this fix, a remote peer that sends an offer with one or more
rejected m-lines will get an answer whose m-line indices are shifted,
causing subsequent re-offers to mis-map transceivers.
Test Plan
cargo build -p rtcpasses (verified)rejected_audio_mline_has_port_zero,rejected_application_mline_uses_sctp_proto,rejected_mline_not_added_to_bundletest_sdp_answer_rejects_audio_correctlycargo fmt,cargo clippy,cargo testall pass cleanReview Feedback Addressed
port==0(not direction) for rejection detection (Copilot comment 3)m=applicationwithUDP/DTLS/SCTP+webrtc-datachannel(Copilot comment 2)candidates_addedtracking (Copilot comment 1)🤖 Generated with Claude Code