Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inactive tracks should not be stopped #2226

Open
k0nserv opened this issue May 18, 2022 · 18 comments
Open

Inactive tracks should not be stopped #2226

k0nserv opened this issue May 18, 2022 · 18 comments

Comments

@k0nserv
Copy link

k0nserv commented May 18, 2022

Your environment.

  • Version: 3.1.0
  • Browser: Chrome
  • Other Information - N/A

What did you do?

  1. Create a track on the browser side
  2. Negotiate the track with Pion
  3. Remove the track on the browser side with removeTrack
  4. Negotiate with Pion

What did you expect?

For Pion to change the direction of the transceiver in question to inactive and not stop it.

What happened?

Pion changes the direction of the transceiver in question to inactive and stops it:

webrtc/peerconnection.go

Lines 1058 to 1061 in ea7964c

} else if direction == RTPTransceiverDirectionInactive {
if err := t.Stop(); err != nil {
return err
}

This relates to the issue webrtc-rs/webrtc#192 which in turns is related to webrtc-rs/webrtc#191 not sure if this happens in Pion too). I think it might also be related to #1843

My understanding of the specification is that the transceiver shouldn't be stopped when this happens.

@adriancable
Copy link
Contributor

@k0nserv - if it's correct that changing the direction to inactive on the Pion side shouldn't stop the track (and this sounds quite plausible, but I haven't checked the RFC) then I think removing L1058-1061 in webrtc/peerconnection.go is necessary but not sufficient.

We would also need to introduce the concept, I suppose, of a 'paused' track, whereby received RTP (but not RTCP?) data is discarded, and sending RTP (but not RTCP?) fails. We'd also need some way of aborting an in-progress ReadRTP and WriteRTP in that track if it enters the pause state while in the middle of those calls. I don't quite have the right mental model yet of the cleanest way to do that.

Thoughts from some experts on this? @davidzhao maybe?

@davidzhao
Copy link
Member

Far from being an expert on this, but I think the current behavior is actually correct.

RTPTransceiver.Stop() in Pion doesn't work exactly the same as the browser. The current implementation of Stop is more like a paused state. According to the specs, once a transceiver is paused, it cannot be restarted. In Pion, it'd happily re-use the same transceiver in order to send other tracks.

@k0nserv
Copy link
Author

k0nserv commented May 21, 2022

I posted a suggestion for a paused state, for the sender, in the webrtc-rs issue. I've not got a sophisticated test bed for Pion, but in webrtc-rs if an m= section becomes inactive it can never transition back to something else as far as I can see. Does that apply to Pion too?

RTPTransceiver.Stop() in Pion doesn't work exactly the same as the browser. The current implementation of Stop is more like a paused state. According to the specs, once a transceiver is paused, it cannot be restarted. In Pion, it'd happily re-use the same transceiver in order to send other tracks.

For example, does this mean you've experienced the transition from inactive back to e.g. sendrecv?

I also noticed that the Stop method doesn't actually update the stopped bool value, in fact I don't see any code path that touch that value. The endless negotiation issue in webrtc-rs stems from this stopping behaviour.

It's also worth noting that the doc comment for Stop says

Stop irreversibly stops the RTPTransceiver

(Emphasis mine)

@davidzhao
Copy link
Member

@k0nserv I believe you are right. I got a bit confused because the behavior is different depending on if it's sending or receiving.

When sending, Pion handles transceiver re-use correctly. RemoveTrack puts the transceiver into an inactive state, until another Track is attached to it.

When receiving, Pion would stop the transceiver if a remote offerer has set the transceiver to inactive. Once stopped, a transceiver cannot be reused again (the comment is correct, even though we seemed to missed setting stopped flag). I think all we need to do here is to remove that Stop and instead set transceiver direction to inactive. PeerConnection.configureRTPReceivers should stop and reset the receiver.

@adriancable
Copy link
Contributor

@davidzhao

I think all we need to do here is to remove that Stop and instead set transceiver direction to inactive.

Which Stop are you referring to? Is it the same as the one @k0nserv originally referred to?

webrtc/peerconnection.go

Lines 1058 to 1061 in ea7964c

} else if direction == RTPTransceiverDirectionInactive {
if err := t.Stop(); err != nil {
return err
}

This only gets called if direction == RTPTransceiverDirectionInactive already, so can you clarify what you mean when you say 'instead' set transceiver direction to inactive.

In summary, I think my question is: do you think removing L1058-L1061 is a necessary and sufficient change to fix this?

@k0nserv
Copy link
Author

k0nserv commented May 22, 2022

In summary, I think my question is: do you think removing L1058-L1061 is a necessary and sufficient change to fix this?

I tested this in webrtc-rs, but it doesn't cause the sender to start sending again when the transceiver in question transitions from inactive to sendonly or sendrecv. Likely, there's some more work that's needed

@davidzhao
Copy link
Member

davidzhao commented May 22, 2022

Which Stop are you referring to? Is it the same as the one @k0nserv originally referred to?

Yes, I'm referring to the same Stop

webrtc/peerconnection.go

Lines 1058 to 1061 in ea7964c

} else if direction == RTPTransceiverDirectionInactive {
if err := t.Stop(); err != nil {
return err
}

This only gets called if direction == RTPTransceiverDirectionInactive already, so can you clarify what you mean when you say 'instead' set transceiver direction to inactive.

That's the direction as specified in the offer, not what the local Transceiver indicates. I'm saying in this case, we need to match the local Transceiver's direction to inactive as well. It'll then trigger a receiver reset in configureRTPReceivers

In summary, I think my question is: do you think removing L1058-L1061 is a necessary and sufficient change to fix this?

@adriancable
Copy link
Contributor

@davidzhao - so something like this?

  1. Remove L1058-1061 in webrtc/peerconnection.go
  2. At L1104 insert:
			case direction == RTPTransceiverDirectionInactive:
				t.setDirection(RTPTransceiverDirectionInactive)

@davidzhao
Copy link
Member

Yeah, I think that should be sufficient.. but this chain of events is sort of complex.. wdyt @Sean-Der ?

@k0nserv
Copy link
Author

k0nserv commented May 24, 2022

I've been experimenting with this in webrtc-rs and while these changes do correct the direction when moving from inactive and back the receiver ends up being stopped anyway. This happens in start_rtp. The reason for this is that received_needs_stopped ends up being true and the receiver is then stopped.

However, this might be different for Pion due to the changes from #2087.

Another question on my mind is: How should it be detected when a transceiver transitions from .e.g. recvonly to inactive? In an SFU case this event must lead to downstream consequence for all other tracks that are sending this data. Is firing the mute event on the track appropriate?

@Sean-Der Do you have peer-to-peer playgrounds for testing with Chrome that you could share as starting point to explore this behaviour?

@k0nserv
Copy link
Author

k0nserv commented May 24, 2022

I've been playing around a bit with this in Chrome using the following playgrounds:

When the transceiver transitions from recvonly in the Receiver to inactive it does fire mute on the track in question as I speculated above. However, going back from inactive to sendonly or sendrecv is not working. On the Sender side explicitly setting sendonly or sendrecv causes the offer that we then create to have a recvonly direction for some reason. I don't know why that this is, but perhaps it's because I don't create the transceiver manually.

@davidzhao
Copy link
Member

Another question on my mind is: How should it be detected when a transceiver transitions from .e.g. recvonly to inactive? In an SFU case this event must lead to downstream consequence for all other tracks that are sending this data. Is firing the mute event on the track appropriate?

In Pion, when configureRTPReceivers is called, it will stop the receiver and cause EOF to be sent to consumer of the track. I don't know if we need to fire any other events that way. mute is fired when there isn't data coming through, but should not be fired when the sender has negotiated the transceiver to inactive.

@k0nserv
Copy link
Author

k0nserv commented May 25, 2022

In Pion, when configureRTPReceivers is called, it will stop the receiver and cause EOF to be sent to consumer of the track. I don't know if we need to fire any other events that way. mute is fired when there isn't data coming through, but should not be fired when the sender has negotiated the transceiver to inactive.

Should that happen though? My understanding is that, just as the transceiver shouldn't be stopped when direction becomes inactive, the receiver and sender should also not be stopped? The spec says that RTCP should continue to be sent but RTP media should stop.

If the direction attribute in the answer indicates that the
JSEP implementation should not be sending media ("recvonly" for
local answers, "sendonly" for remote answers, or "inactive"
either type of answer), stop transmitting all RTP media, but
continue sending RTCP, as described in [RFC3264], Section 5.1.

From RFC3264 Section 5.1

If the offerer wishes to only receive media
from its peer, it MUST mark the stream as recvonly. If the offerer
wishes to communicate, but wishes to neither send nor receive media
at this time, it MUST mark the stream with an "a=inactive" attribute.
The inactive direction attribute is specified in RFC 3108 [3]. Note
that in the case of the Real Time Transport Protocol (RTP) [4], RTCP
is still sent and received for sendonly, recvonly, and inactive
streams. That is, the directionality of the media stream has no
impact on the RTCP usage. If the offerer wishes to both send and
receive media with its peer, it MAY include an "a=sendrecv"
attribute, or it MAY omit it, since sendrecv is the default.

I'm also curious about how you handle this in LiveKit, i.e. when a camera track is removed how is that handled?

@davidzhao
Copy link
Member

The spec says that RTCP should continue to be sent but RTP media should stop.

You are right. though I'm not sure what type of RTCP it would still make sense to send. The receiving end would have marked this track as completed. When the sender wishes to send another track, it would trigger the receiver to fire onTrack again.

I'm also curious about how you handle this in LiveKit, i.e. when a camera track is removed how is that handled?

Within LiveKit, the behavior differs depending on the platform. When a receiving browser sees a transceiver go from recvonly to inactive, the MediaStreamTrack is removed from MediaStream. we handle it here.

On mobile platforms, it would notify us that RTPReceiver had been removed. here's how we handle it.

The behavior on mobile seem to indicate that the RTPReceiver should be removed/stopped entirely.

@k0nserv
Copy link
Author

k0nserv commented Jun 6, 2022

Hmm, right I suppose the receiver can be removed and re-created entirely instead of entering some paused state and then resuming. However, since the specification says that RTCP should continue to flow that seems to indicate that it's decoupled from the RTCPReceiver in libWebrtc then.

@k0nserv
Copy link
Author

k0nserv commented Jun 6, 2022

FWIW there's this test in libWebrtc that asserts that the transceivers are not stopped in the case of an m= section becoming inactive.

@k0nserv
Copy link
Author

k0nserv commented Jun 6, 2022

Details from the spec in https://w3c.github.io/webrtc-pc/#set-the-session-description in step 7.9.1.7.2. Implementation in libWebRTC

@langhuihui
Copy link

so how can I do to receive data on the server side when inactive back to sendrecv on the browser side ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants