fix(rtp_transceiver): trigger renegotiation on set_direction (closes #51) by nightness · Pull Request #71 · webrtc-rs/rtc

nightness · 2026-04-01T11:12:44Z

Summary

RTCRtpTransceiver::set_direction() now calls trigger_negotiation_needed() on the peer connection when the direction actually changes, per W3C WebRTC §5.5
The fix uses a narrow pub(crate) on_transceiver_direction_changed() method to keep trigger_negotiation_needed at pub(super) visibility
Compares the transceiver's effective direction after the setter (not the input value) for robustness against future normalization
The trace! log is moved from internal.rs to mod.rs where the negotiation logic lives
The now-unnecessary previous_direction variable in internal.rs is removed
Removed obsolete //TODO: #[cfg(test)] mod rtp_transceiver_test; comment since inline tests now exist

Review feedback addressed

rainliu: Move trace! from internal set_direction to public set_direction (commit 731341b)
rainliu: Add unit test and integration test (commits 539b935, e17adf6)
Copilot: Compare effective direction after set_direction, not input (commit e17adf6)
Copilot: Narrow pub(crate) exposure via on_transceiver_direction_changed() (commit 2252d20)
Copilot: Remove misleading TODO comment for rtp_transceiver_test module (commit 8d816eb)

Test plan

cargo build -p rtc passes
cargo test -p rtc passes (246 lib tests + 28 integration tests, 0 failures)
cargo fmt --check clean
cargo clippy clean
Unit test: test_set_direction_triggers_negotiation_on_change — verifies OnNegotiationNeededEvent is emitted when direction changes
Unit test: test_set_direction_noop_when_unchanged — verifies no event when setting the same direction
Integration test: test_set_direction_change_triggers_renegotiation — full offer/answer cycle via public Protocol API with Opus codecs, STUN server, and host candidates; asserts poll_event() returns OnNegotiationNeededEvent on direction change and no event on same-direction set

Generated with Claude Code

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates transceiver direction changes to correctly trigger WebRTC renegotiation, aligning behavior with W3C WebRTC §5.5 and closing #51.

Changes:

Trigger negotiationneeded when RTCRtpTransceiver::set_direction() actually changes the direction.
Remove the resolved TODO from the internal transceiver implementation.
Broaden trigger_negotiation_needed() visibility to enable calling it from the transceiver wrapper.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
rtc/src/rtp_transceiver/mod.rs	Triggers negotiation-needed on actual direction changes in the public wrapper.
rtc/src/rtp_transceiver/internal.rs	Removes obsolete TODO/comments after behavior is implemented elsewhere.
rtc/src/peer_connection/internal.rs	Expands helper visibility to allow cross-module invocation within the crate.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-04-04T14:14:01Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.17%. Comparing base (9feb4a3) to head (6ad88b6).

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #71   +/-   ##
=======================================
  Coverage   71.17%   71.17%           
=======================================
  Files         442      442           
  Lines       67330    67330           
=======================================
+ Hits        47922    47925    +3     
+ Misses      19408    19405    -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

rainliu

since this involves public API's behavior change, please add unit test and integration test to verify negotiation is actually trigged when direction is changed.

nightness · 2026-04-07T16:04:42Z

Moved the trace! log from internal.rs::set_direction() to mod.rs::set_direction() — the internal setter doesn't trigger negotiation, so the trace belongs in the public wrapper where the actual negotiation logic lives. Also removed the now-unnecessary previous_direction variable and empty if block from the internal setter.

nightness · 2026-04-07T16:40:42Z

Addressed Copilot review comments:

Visibility: Kept trigger_negotiation_needed as pub(super). Added a narrow pub(crate) on_transceiver_direction_changed() method that the transceiver wrapper calls, limiting the crate-visible surface to a purpose-specific API.
Tests: Added two unit tests:
- test_set_direction_triggers_negotiation_on_change — verifies OnNegotiationNeededEvent is emitted when direction changes
- test_set_direction_noop_when_unchanged — verifies no event is emitted when setting the same direction

nightness · 2026-04-07T17:06:25Z

Added integration test (tests/renegotiation_set_direction.rs) that verifies through the public Protocol API:

Direction change triggers OnNegotiationNeededEvent — creates two peers with Opus codecs and STUN config, completes a full offer/answer cycle, then changes direction from Recvonly to Inactive and asserts the event fires via poll_event()
Same direction is a no-op — setting Inactive again produces no event

This complements the existing unit tests which verify the same behavior at the internal level.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ebrtc-rs#51) Per W3C WebRTC §5.5, changing an RTCRtpTransceiver's direction must trigger the negotiation-needed flag so the application knows to create a new offer/answer exchange. The internal set_direction() had a TODO noting this was missing. The fix lives in the public RTCRtpTransceiver::set_direction() wrapper, which already holds &mut RTCPeerConnection and can call trigger_negotiation_needed() directly after detecting a direction change. Also broaden trigger_negotiation_needed() visibility from pub(super) to pub(crate) so rtp_transceiver/mod.rs can reach it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…irection The internal set_direction() doesn't trigger negotiation, so the trace belongs in the public wrapper (mod.rs) where the actual negotiation logic lives. This also removes the now-unnecessary previous_direction variable from the internal setter. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Keep trigger_negotiation_needed as pub(super); expose a narrow pub(crate) on_transceiver_direction_changed() method instead of broadening the general helper's visibility - Add tests verifying (1) changing direction triggers OnNegotiationNeededEvent and (2) setting the same direction is a no-op Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Verifies through the public Protocol API that: 1. Changing direction triggers OnNegotiationNeededEvent 2. Setting the same direction is a no-op (no event) Uses a full offer/answer cycle with registered Opus codecs, STUN server config, and host candidates to properly initialize the negotiation state machine before testing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Compare the transceiver's actual direction after the setter rather than the requested value. This is more robust if set_direction ever normalizes or refuses a value in the future. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

The inline #[cfg(test)] mod tests block now exists in this file, making the commented-out TODO for a separate test module misleading. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

- Replace `use sansio::Protocol` with `use rtc::sansio::Protocol` for clarity (the import IS needed for poll_event()) - Replace hard-coded UDP ports 10000/10001 with OS-assigned ephemeral ports via 127.0.0.1:0 binding - Rewrite unit tests to use offer/answer cycle + poll_event() instead of internal state manipulation (pipeline_context.event_outs.clear() and reset_negotiation_state()) - Remove test-only reset_negotiation_state() helper from RTCPeerConnection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

nightness · 2026-04-10T05:25:58Z

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.

nightness force-pushed the fix/set-direction-renegotiation branch from e08e980 to 6ad88b6 Compare April 1, 2026 18:43

rainliu requested a review from Copilot April 4, 2026 14:06

Copilot AI reviewed Apr 4, 2026

View reviewed changes

Comment thread rtc/src/rtp_transceiver/mod.rs Outdated

Comment thread rtc/src/peer_connection/internal.rs Outdated

Comment thread rtc/src/rtp_transceiver/mod.rs Outdated

Copilot started reviewing on behalf of rainliu April 4, 2026 14:16 View session

rainliu reviewed Apr 4, 2026

View reviewed changes

Comment thread rtc/src/rtp_transceiver/internal.rs Outdated

rainliu requested changes Apr 4, 2026

View reviewed changes

nightness requested a review from Copilot April 7, 2026 17:18

Copilot started reviewing on behalf of nightness April 7, 2026 17:18 View session

Copilot AI reviewed Apr 7, 2026

View reviewed changes

Comment thread rtc/src/rtp_transceiver/mod.rs Outdated

nightness requested review from Copilot and rainliu April 8, 2026 07:58

Copilot started reviewing on behalf of nightness April 8, 2026 07:59 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

Comment thread rtc/tests/renegotiation_set_direction.rs Outdated

Comment thread rtc/tests/renegotiation_set_direction.rs Outdated

Comment thread rtc/src/rtp_transceiver/mod.rs Outdated

Comment thread rtc/src/peer_connection/mod.rs Outdated

nightness requested a review from Copilot April 8, 2026 09:48

Copilot started reviewing on behalf of nightness April 8, 2026 09:49 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

nightness and others added 7 commits April 10, 2026 00:15

remove obsolete TODO comment for rtp_transceiver_test module

a17d250

The inline #[cfg(test)] mod tests block now exists in this file, making the commented-out TODO for a separate test module misleading. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

nightness force-pushed the fix/set-direction-renegotiation branch from 893f362 to 2d05d1d Compare April 10, 2026 05:15

nightness mentioned this pull request Apr 10, 2026

Summary: 19 PRs ready for review #87

Open

Uh oh!

Conversation

nightness commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Review feedback addressed

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Apr 4, 2026

Codecov Report

Uh oh!

Uh oh!

rainliu left a comment

Choose a reason for hiding this comment

Uh oh!

nightness commented Apr 7, 2026

Uh oh!

nightness commented Apr 7, 2026

Uh oh!

nightness commented Apr 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

nightness commented Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nightness commented Apr 1, 2026 •

edited

Loading