feat: delete rejected transceiver #3007

o-u-p · 2025-01-16T10:23:39Z

when port = 0 and direction = inactive indicates that the media stream is not wanted, so we need delete transceiver, sync the logic with c++ implement
https://www.ietf.org/rfc/rfc3264.txt#:~:text=A%20port%20number%20of%20zero,rejected%20stream%20(Section%206).

codecov · 2025-01-16T11:28:33Z

Codecov Report

Attention: Patch coverage is 80.39216% with 10 lines in your changes missing coverage. Please review.

Project coverage is 79.66%. Comparing base (49b555b) to head (ea155fa).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
peerconnection.go	76.74%	7 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3007      +/-   ##
==========================================
+ Coverage   77.96%   79.66%   +1.70%     
==========================================
  Files          89       78      -11     
  Lines       10578     9728     -850     
==========================================
- Hits         8247     7750     -497     
+ Misses       1840     1533     -307     
+ Partials      491      445      -46

Flag	Coverage Δ
go	`79.66% <80.39%> (+0.12%)`	⬆️
wasm	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Sean-Der · 2025-01-16T14:32:26Z

Thank you so much @o-u-p

Would you be able to write a test for this? If you could have a static answer and then just show that disabling works.

JoeTurki · 2025-01-16T15:14:51Z

Correct me if I'm wrong, but shouldn't this be handled in setLocalDescription rather than in createAnswer?

Sean-Der · 2025-01-16T17:00:25Z

@joeturki you are right! Actual changes/committing sound only happen in set*Description

KevinVison · 2025-01-17T06:19:45Z

Correct me if I'm wrong, but shouldn't this be handled in setLocalDescription rather than in createAnswer?

you're right, I have update the code and add example page. the unit test may not be easy, since the signal state will be have-remote-offer->SetRemote(offer)->have-remote-offer

JoeTurki

I'm not sure about the side effects of automatically deleting transceivers like this, Also I'm not sure if this follows the spec on this, It would help us to have few tests for this behavior:

If any RtpTransceiver has been added, and there exists an m= section with a zero port in the current local description or the current remote description, that m= section MUST be recycled by generating an m= section for the added RtpTransceiver as if the m= section were being added to the session description (including a new MID value), and placing it at the same index as the m= section with a zero port.

If an RtpTransceiver is stopped and is not associated with an m= section, an m= section MUST NOT be generated for it. This prevents adding back RtpTransceivers whose m= sections were recycled and used for a new RtpTransceiver in a previous offer/ answer exchange, as described above.

If an RtpTransceiver has been stopped and is associated with an m= section, and the m= section is not being recycled as described above, an m= section MUST be generated for it with the port set to zero and all "a=msid" lines removed.

https://rtcweb-wg.github.io/jsep/#sec.subsequent-offers

Thank you for your work, let me know if you need help writing tests :)

(Sorry for the long review)

JoeTurki · 2025-01-17T06:44:31Z

examples/delete-transceiver/README.md

@@ -0,0 +1,18 @@
+# delete-transceiver


I don't think we can add an example for this, At least in this PR!

I add the example to clarify the usage, please let me know if you have any concern

I'm not sure if adding a standalone example for a spec behavior change is appropriate. Also, "delete transceivers" means something different from handling of "rejected transceivers." Perhaps just adding a few unit tests would be sufficient?

That said, I'm open to hearing other opinions on this, and maybe wait for others!

I try to add unit test, I cannot set multiple times of remote or local description in the unit test

Wdym, like renegotiation? there are few unit tests you can base of.

yep, maybe hard to write

JoeTurki · 2025-01-17T06:44:58Z

peerconnection.go

@@ -1007,14 +1006,18 @@ func (pc *PeerConnection) SetLocalDescription(desc SessionDescription) error {
 	weAnswer := desc.Type == SDPTypeAnswer
 	remoteDesc := pc.RemoteDescription()
 	if weAnswer && remoteDesc != nil {
-		_ = setRTPTransceiverCurrentDirection(&desc, currentTransceivers, false)
+		rejected := []string{}
+		_ = setRTPTransceiverCurrentDirection(&desc, currentTransceivers, false, &rejected)


Can we do this without using a pointer?

I add pointer here to make the logic concise. 1. the first offer/answer doesn't need this, so when renegotiate the rejected would not be nil; 2. I want to get the rejected mid value but keep original func return just error

I'm sorry but this style isn't idiomatic, and it makes the code harder to maintain in the future, also I'm not sure why don't just delete rejected transceivers is there a reason why you extract the mids first?

it may be more efficient

JoeTurki · 2025-01-17T06:45:20Z

peerconnection.go

@@ -1184,14 +1187,18 @@ func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {

 	if isRenegotiation {
 		if weOffer {
-			_ = setRTPTransceiverCurrentDirection(&desc, currentTransceivers, true)
+			rejected := []string{}
+			_ = setRTPTransceiverCurrentDirection(&desc, currentTransceivers, true, &rejected)


Can we do this without using a pointer?

same as above

JoeTurki · 2025-01-17T06:45:33Z

peerconnection.go

@@ -1214,7 +1221,7 @@ func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {
 	// Start the networking in a new routine since it will block until
 	// the connection is actually established.
 	if weOffer {
-		_ = setRTPTransceiverCurrentDirection(&desc, currentTransceivers, true)
+		_ = setRTPTransceiverCurrentDirection(&desc, currentTransceivers, true, nil)


JoeTurki · 2025-01-17T06:46:26Z

peerconnection.go

+		needDelete := false
+		for _, mid := range mids {
+			if transceiver.Mid() == mid {
+				transceiver.Stop()


We should log this error!

JoeTurki · 2025-01-17T06:47:04Z

peerconnection.go

+// removeRTPTransceiver remove inactive
+// and fires onNegotiationNeeded;
+// caller of this method should hold `pc.mu` lock
+func (pc *PeerConnection) removeRTPTransceiver(mids []string) {


Can we handle the recycling in single loop?

I can use a map to make the time complexity to o(n)

JoeTurki · 2025-01-17T06:52:28Z

peerconnection.go

+			n++
+		}
+	}
+	pc.rtpTransceivers = pc.rtpTransceivers[:n]


What if we delete a transceiver from the middle of the slice? Can you add a unit test for this?

JoeTurki · 2025-01-17T06:52:43Z

peerconnection.go

+		}
+	}
+	pc.rtpTransceivers = pc.rtpTransceivers[:n]
+	pc.onNegotiationNeeded()


What is the spec for calling onNegotiationNeeded() here?

transceiver modify or restart ice, etc need to call onNegotiationNeeded, but this func is not export outside, I will remove it for unnecessary

We shouldn't trigger onNegotiationNeeded() from setLocalDescription.

If an RtpTransceiver has been stopped and is associated with an m= section, and the m= section is not being recycled as described above, an m= section MUST be generated for it with the port set to zero and all "a=msid" lines removed.

JoeTurki · 2025-01-17T06:54:03Z

peerconnection.go

-
+		// reject transceiver if it is inactive
+		if rejected != nil && media.MediaName.Port.Value == 0 && direction == RTPTransceiverDirectionInactive {
+			*rejected = append(*rejected, midValue)


Can we do this without using a pointer?

o-u-p · 2025-01-17T10:10:47Z

I'm not sure about the side effects of automatically deleting transceivers like this, Also I'm not sure if this follows the spec on this, It would help us to have few tests for this behavior:

If any RtpTransceiver has been added, and there exists an m= section with a zero port in the current local description or the current remote description, that m= section MUST be recycled by generating an m= section for the added RtpTransceiver as if the m= section were being added to the session description (including a new MID value), and placing it at the same index as the m= section with a zero port.

If an RtpTransceiver is stopped and is not associated with an m= section, an m= section MUST NOT be generated for it. This prevents adding back RtpTransceivers whose m= sections were recycled and used for a new RtpTransceiver in a previous offer/ answer exchange, as described above.

If an RtpTransceiver has been stopped and is associated with an m= section, and the m= section is not being recycled as described above, an m= section MUST be generated for it with the port set to zero and all "a=msid" lines removed.

https://rtcweb-wg.github.io/jsep/#sec.subsequent-offers

Thank you for your work, let me know if you need help writing tests :)

(Sorry for the long review)

see SdpOfferAnswerHandler::RemoveStoppedTransceivers in c++
`
void SdpOfferAnswerHandler::RemoveStoppedTransceivers() {
TRACE_EVENT0("webrtc", "SdpOfferAnswerHandler::RemoveStoppedTransceivers");

RTC_DCHECK_RUN_ON(signaling_thread());
// 3.2.10.1: For each transceiver in the connection's set of transceivers
// run the following steps:
if (!IsUnifiedPlan())
return;
if (!ConfiguredForMedia()) {
return;
}
// Traverse a copy of the transceiver list.
auto transceiver_list = transceivers()->List();
for (auto transceiver : transceiver_list) {
// 3.2.10.1.1: If transceiver is stopped, associated with an m= section
// and the associated m= section is rejected in
// connection.[[CurrentLocalDescription]] or
// connection.[[CurrentRemoteDescription]], remove the
// transceiver from the connection's set of transceivers.
if (!transceiver->stopped()) {
continue;
}
const ContentInfo* local_content = FindMediaSectionForTransceiver(
transceiver->internal(), local_description());
const ContentInfo* remote_content = FindMediaSectionForTransceiver(
transceiver->internal(), remote_description());
if ((local_content && local_content->rejected) ||
(remote_content && remote_content->rejected)) {
RTC_LOG(LS_INFO) << "Dissociating transceiver"
" since the media section is being recycled.";
transceiver->internal()->set_mid(std::nullopt);
transceiver->internal()->set_mline_index(std::nullopt);
} else if (!local_content && !remote_content) {
// TODO(bugs.webrtc.org/11973): Consider if this should be removed already
// See w3c/webrtc-pc#2576
RTC_LOG(LS_INFO)
<< "Dropping stopped transceiver that was never associated";
}
transceivers()->Remove(transceiver);
}
}
`

JoeTurki · 2025-01-17T10:39:54Z

I'm talking about side effects from deleting transceivers this way in Pion, we should make sure that.

Recycling of m= sections: Unused m= sections should be available for other transceivers.
Pion should support recycling m= sections for transceivers associated from other clients.
If a transceiver is not recycled and is still associated with a mid section, we should generate a m= section for it with the port set to zero and all a=msid lines removed.

I believe the snippet you referenced is from libwebrtc, and I'm confident libwebrtc implements behavior.

KevinVison · 2025-01-18T05:10:51Z

I'm talking about side effects from deleting transceivers this way in Pion, we should make sure that.

Recycling of m= sections: Unused m= sections should be available for other transceivers.

Pion should support recycling m= sections for transceivers associated from other clients.

If a transceiver is not recycled and is still associated with a mid section, we should generate a m= section for it with the port set to zero and all a=msid lines removed.

I believe the snippet you referenced is from libwebrtc, and I'm confident libwebrtc implements behavior.

I think it's the original logic? and I just delete the transceiver that is not wanted exactly, if there are unused transceiver,it will still use the reuse track logic as before.
I think so, but I think my implement is standard and should be suitable for other clients.
I think the original logic will do this, correct me if I'm wrong

o-u-p force-pushed the feat/delete_transceiver branch 2 times, most recently from 1932439 to 0ea417e Compare January 16, 2025 11:10

o-u-p force-pushed the feat/delete_transceiver branch from ee016ad to 4e29ef2 Compare January 17, 2025 06:13

o-u-p force-pushed the feat/delete_transceiver branch from 4e29ef2 to b0bca58 Compare January 17, 2025 06:38

JoeTurki requested changes Jan 17, 2025

View reviewed changes

o-u-p force-pushed the feat/delete_transceiver branch from b0bca58 to bd02493 Compare January 17, 2025 10:08

Delete rejected transceiver

ea155fa

o-u-p force-pushed the feat/delete_transceiver branch from bd02493 to ea155fa Compare January 18, 2025 05:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: delete rejected transceiver #3007

feat: delete rejected transceiver #3007

o-u-p commented Jan 16, 2025

codecov bot commented Jan 16, 2025 •

edited

Loading

Sean-Der commented Jan 16, 2025

JoeTurki commented Jan 16, 2025

Sean-Der commented Jan 16, 2025

KevinVison commented Jan 17, 2025 •

edited

Loading

JoeTurki left a comment

JoeTurki Jan 17, 2025

KevinVison Jan 17, 2025

JoeTurki Jan 17, 2025

o-u-p Jan 17, 2025

JoeTurki Jan 17, 2025

KevinVison Jan 18, 2025

JoeTurki Jan 17, 2025

KevinVison Jan 17, 2025

JoeTurki Jan 17, 2025

KevinVison Jan 18, 2025

JoeTurki Jan 17, 2025

KevinVison Jan 17, 2025

JoeTurki Jan 17, 2025

KevinVison Jan 17, 2025

JoeTurki Jan 17, 2025

KevinVison Jan 18, 2025

JoeTurki Jan 17, 2025

o-u-p Jan 17, 2025

JoeTurki Jan 17, 2025

KevinVison Jan 18, 2025

JoeTurki Jan 17, 2025

o-u-p Jan 17, 2025

JoeTurki Jan 17, 2025

JoeTurki Jan 17, 2025

o-u-p Jan 17, 2025

o-u-p commented Jan 17, 2025 •

edited

Loading

JoeTurki commented Jan 17, 2025

KevinVison commented Jan 18, 2025

feat: delete rejected transceiver #3007

Are you sure you want to change the base?

feat: delete rejected transceiver #3007

Conversation

o-u-p commented Jan 16, 2025

codecov bot commented Jan 16, 2025 • edited Loading

Codecov Report

Sean-Der commented Jan 16, 2025

JoeTurki commented Jan 16, 2025

Sean-Der commented Jan 16, 2025

KevinVison commented Jan 17, 2025 • edited Loading

JoeTurki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

o-u-p commented Jan 17, 2025 • edited Loading

JoeTurki commented Jan 17, 2025

KevinVison commented Jan 18, 2025

codecov bot commented Jan 16, 2025 •

edited

Loading

KevinVison commented Jan 17, 2025 •

edited

Loading

o-u-p commented Jan 17, 2025 •

edited

Loading