diff --git a/pkg/rtc/participant.go b/pkg/rtc/participant.go index 04ba5eef0..f7e9937a1 100644 --- a/pkg/rtc/participant.go +++ b/pkg/rtc/participant.go @@ -158,7 +158,6 @@ type ParticipantParams struct { ForwardStats *sfu.ForwardStats DisableSenderReportPassThrough bool MetricConfig metric.MetricConfig - DropRemoteICECandidates bool } type ParticipantImpl struct { @@ -1449,7 +1448,6 @@ func (p *ParticipantImpl) setupTransportManager() error { PublisherHandler: pth, SubscriberHandler: sth, DataChannelStats: p.dataChannelStats, - DropRemoteICECandidates: p.params.DropRemoteICECandidates, } if p.params.SyncStreams && p.params.PlayoutDelay.GetEnabled() && p.params.ClientInfo.isFirefox() { // we will disable playout delay for Firefox if the user is expecting diff --git a/pkg/rtc/transport.go b/pkg/rtc/transport.go index df7ad0d61..fba7f6c51 100644 --- a/pkg/rtc/transport.go +++ b/pkg/rtc/transport.go @@ -234,8 +234,6 @@ type PCTransport struct { connectionDetails *types.ICEConnectionDetails selectedPair atomic.Pointer[webrtc.ICECandidatePair] - - dropRemoteICECandidates bool } type TransportParams struct { @@ -256,7 +254,6 @@ type TransportParams struct { IsSendSide bool AllowPlayoutDelay bool DataChannelMaxBufferedAmount uint64 - DropRemoteICECandidates bool } func newPeerConnection(params TransportParams, onBandwidthEstimator func(estimator cc.BandwidthEstimator)) (*webrtc.PeerConnection, *webrtc.MediaEngine, error) { @@ -301,9 +298,12 @@ func newPeerConnection(params TransportParams, onBandwidthEstimator func(estimat se.DisableSRTCPReplayProtection(true) if !params.ProtocolVersion.SupportsICELite() || !params.ClientInfo.SupportPrflxOverRelay() { // if client don't support prflx over relay which is only Firefox, disable ICE Lite to ensure that - // dropping remote ICE candidates does not get enabled. Firefox does aggressive nomination and - // dropping remote ICE candidates means server would accept all switches and it could end up with - // the lower priority candidate. As Firefox does not support migration, ICE Lite can be disabled. + // aggressive nomination is handled properly. Firefox does aggressive nomination even if peer is + // ICE Lite (see comment as to historical reasons: https://github.com/pion/ice/pull/739#issuecomment-2452245066). + // pion/ice (as of v2.3.37) will accept all use-candidate switches when in ICE Lite mode. + // That combined with aggressive nomination from Firefox could potentially lead to the two ends + // ending up with different candidates. + // As Firefox does not support migration, ICE Lite can be disabled. se.SetLite(false) } se.SetDTLSRetransmissionInterval(dtlsRetransmissionInterval) @@ -1437,12 +1437,6 @@ func (t *PCTransport) handleRemoteICECandidate(e event) error { return nil } - if t.dropRemoteICECandidates && strings.Contains(strings.ToLower(c.Candidate), "srflx") { - t.params.Logger.Debugw("dropping remote ICE candidate", "candidate", c.Candidate) - t.connectionDetails.AddRemoteCandidate(*c, true, true, true) - return nil - } - if err := t.pc.AddICECandidate(*c); err != nil { t.params.Logger.Warnw("failed to add cached ICE candidate", err, "candidate", c) return errors.Wrap(err, "add ice candidate failed") @@ -1467,31 +1461,6 @@ func (t *PCTransport) filterCandidates(sd webrtc.SessionDescription, preferTCP, return sd } - _, iceLite := parsed.Attribute("ice-lite") - var liteSet bool - if isLocal { - if t.localICEIsLite == nil { - t.localICEIsLite = &iceLite - liteSet = true - } - } else { - if t.remoteICEIsLite == nil { - t.remoteICEIsLite = &iceLite - liteSet = true - } - } - if liteSet && t.localICEIsLite != nil && t.remoteICEIsLite != nil { - // only drop remote candidates if local is lite and remote is not - t.dropRemoteICECandidates = t.params.DropRemoteICECandidates && (*t.localICEIsLite && !*t.remoteICEIsLite) - t.params.Logger.Debugw( - "setting DropRemoteICECandidates", - "dropRemoteICECandidatesConfig", t.params.DropRemoteICECandidates, - "dropRemoteICECandidatesCalculated", t.dropRemoteICECandidates, - "localICELite", *t.localICEIsLite, - "remoteICELite", *t.remoteICEIsLite, - ) - } - filterAttributes := func(attrs []sdp.Attribute) []sdp.Attribute { filteredAttrs := make([]sdp.Attribute, 0, len(attrs)) for _, a := range attrs { @@ -1502,7 +1471,7 @@ func (t *PCTransport) filterCandidates(sd webrtc.SessionDescription, preferTCP, filteredAttrs = append(filteredAttrs, a) continue } - excluded := (!isLocal && t.dropRemoteICECandidates && c.Type() == ice.CandidateTypeServerReflexive) || (preferTCP && !c.NetworkType().IsTCP()) + excluded := preferTCP && !c.NetworkType().IsTCP() if !excluded { if !t.params.Config.UseMDNS && types.IsICECandidateMDNS(c) { excluded = true @@ -1720,11 +1689,6 @@ func (t *PCTransport) setRemoteDescription(sd webrtc.SessionDescription) error { } for _, c := range t.pendingRemoteCandidates { - if t.dropRemoteICECandidates && strings.Contains(strings.ToLower(c.Candidate), "srflx") { - t.params.Logger.Debugw("dropping remote ICE candidate (pending)", "candidate", c.Candidate) - t.connectionDetails.AddRemoteCandidate(*c, true, true, true) - continue - } if err := t.pc.AddICECandidate(*c); err != nil { t.params.Logger.Warnw("failed to add cached ICE candidate", err, "candidate", c) return errors.Wrap(err, "add ice candidate failed") diff --git a/pkg/rtc/transport_test.go b/pkg/rtc/transport_test.go index 4cda22e4f..c80b206ef 100644 --- a/pkg/rtc/transport_test.go +++ b/pkg/rtc/transport_test.go @@ -28,7 +28,6 @@ import ( "github.com/livekit/livekit-server/pkg/rtc/transport" "github.com/livekit/livekit-server/pkg/rtc/transport/transportfakes" - "github.com/livekit/livekit-server/pkg/rtc/types" "github.com/livekit/livekit-server/pkg/testutils" "github.com/livekit/protocol/livekit" ) @@ -504,115 +503,6 @@ func TestFilteringCandidates(t *testing.T) { transport.Close() } -func TestDropRemoteICECandidates(t *testing.T) { - cases := []struct { - name string - remoteLite bool - localLite bool - expectedLocalDrop bool - expectedRemoteDrop bool - }{ - { - name: "both not lite", - localLite: false, - remoteLite: false, - expectedLocalDrop: false, - expectedRemoteDrop: false, - }, - { - name: "remote lite", - localLite: false, - remoteLite: true, - expectedLocalDrop: false, - expectedRemoteDrop: true, - }, - { - name: "local lite", - localLite: true, - remoteLite: false, - expectedLocalDrop: true, - expectedRemoteDrop: false, - }, - { - name: "both lite", - localLite: true, - remoteLite: true, - expectedLocalDrop: false, - expectedRemoteDrop: false, - }, - } - - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - params := TransportParams{ - ParticipantID: "id", - ParticipantIdentity: "identity", - Config: &WebRTCConfig{}, - IsOfferer: true, - ProtocolVersion: types.CurrentProtocol, - DropRemoteICECandidates: true, - } - - paramsA := params - paramsA.Config.SettingEngine.SetLite(c.localLite) - handlerA := &transportfakes.FakeHandler{} - paramsA.Handler = handlerA - transportLocal, err := NewPCTransport(paramsA) - require.NoError(t, err) - _, err = transportLocal.pc.CreateDataChannel(LossyDataChannel, nil) - require.NoError(t, err) - - paramsB := params - paramsB.Config.SettingEngine.SetLite(c.remoteLite) - handlerB := &transportfakes.FakeHandler{} - paramsB.Handler = handlerB - paramsB.IsOfferer = false - transportRemote, err := NewPCTransport(paramsB) - require.NoError(t, err) - - require.False(t, transportLocal.IsEstablished()) - require.False(t, transportRemote.IsEstablished()) - - handleICEExchange(t, transportLocal, transportRemote, handlerA, handlerB) - var offer atomic.Pointer[webrtc.SessionDescription] - handlerA.OnOfferCalls(func(sd webrtc.SessionDescription) error { - parsed, err := sd.Unmarshal() - require.NoError(t, err) - _, lite := parsed.Attribute("ice-lite") - require.Equal(t, c.localLite, lite) - offer.Store(&sd) - return nil - }) - transportLocal.Negotiate(true) - require.Eventually(t, func() bool { - return offer.Load() != nil - }, 100*time.Millisecond, time.Millisecond*10, "offer not received") - - handlerB.OnAnswerCalls(func(sd webrtc.SessionDescription) error { - parsed, err := sd.Unmarshal() - require.NoError(t, err) - _, lite := parsed.Attribute("ice-lite") - require.Equal(t, c.remoteLite, lite, sd.SDP) - transportLocal.HandleRemoteDescription(sd) - return nil - }) - transportRemote.HandleRemoteDescription(*offer.Load()) - - require.Eventually(t, func() bool { - return transportLocal.IsEstablished() - }, 10*time.Second, time.Millisecond*10, "transportA is not established") - require.Eventually(t, func() bool { - return transportRemote.IsEstablished() - }, 10*time.Second, time.Millisecond*10, "transportB is not established") - - require.Equal(t, c.expectedLocalDrop, transportLocal.dropRemoteICECandidates) - require.Equal(t, c.expectedRemoteDrop, transportRemote.dropRemoteICECandidates) - - transportLocal.Close() - transportRemote.Close() - }) - } -} func handleICEExchange(t *testing.T, a, b *PCTransport, ah, bh *transportfakes.FakeHandler) { ah.OnICECandidateCalls(func(candidate *webrtc.ICECandidate, target livekit.SignalTarget) error { diff --git a/pkg/rtc/transportmanager.go b/pkg/rtc/transportmanager.go index bee94e700..cc69e514c 100644 --- a/pkg/rtc/transportmanager.go +++ b/pkg/rtc/transportmanager.go @@ -103,7 +103,6 @@ type TransportManagerParams struct { PublisherHandler transport.Handler SubscriberHandler transport.Handler DataChannelStats *telemetry.BytesTrackStats - DropRemoteICECandidates bool } type TransportManager struct { @@ -158,7 +157,6 @@ func NewTransportManager(params TransportManagerParams) (*TransportManager, erro ClientInfo: params.ClientInfo, Transport: livekit.SignalTarget_PUBLISHER, Handler: TransportManagerPublisherTransportHandler{TransportManagerTransportHandler{params.PublisherHandler, t, lgr}}, - DropRemoteICECandidates: params.DropRemoteICECandidates, }) if err != nil { return nil, err @@ -182,7 +180,6 @@ func NewTransportManager(params TransportManagerParams) (*TransportManager, erro DataChannelMaxBufferedAmount: params.DataChannelMaxBufferedAmount, Transport: livekit.SignalTarget_SUBSCRIBER, Handler: TransportManagerTransportHandler{params.SubscriberHandler, t, lgr}, - DropRemoteICECandidates: params.DropRemoteICECandidates, }) if err != nil { return nil, err