From 35bef35d66bf9b03894d3debc363143a5c2cb60c Mon Sep 17 00:00:00 2001 From: Raja Subramanian Date: Sat, 2 Nov 2024 10:50:55 +0530 Subject: [PATCH] Clean up drop ICE candidates. (#3153) * Clean up drop ICE candidates. With pion/ice v2.3.37, ICE Lite will accept use-candidate from peer. So, there is no need to drop candidates. Still leaving the FF change to not use Lite which was added as part of this effort initially due to how FF does nominations. Updated comment to explain why. * clean up test --- pkg/rtc/participant.go | 2 - pkg/rtc/transport.go | 50 +++------------- pkg/rtc/transport_test.go | 110 ------------------------------------ pkg/rtc/transportmanager.go | 3 - 4 files changed, 7 insertions(+), 158 deletions(-) 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