dandri/livekit
1
0
Fork 0
mirror of https://github.com/livekit/livekit.git synced 2026-05-14 22:45:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

Clean up drop ICE candidates. (#3153)

Browse Source 
* 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
This commit is contained in:
Raja Subramanian
2024-11-02 10:50:55 +05:30
committed by GitHub
parent 150433ab64
commit 35bef35d66
4 changed files with 7 additions and 158 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/rtc/participant.go
-2
View File
@@ -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
pkg/rtc/transport.go
+7 -43
View File
@@ -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")
pkg/rtc/transport_test.go
-110
View File
@@ -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 {
pkg/rtc/transportmanager.go
-3
View File
@@ -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