sfu: don't count opus useinbandfec=0 as FEC in the down track (#5182)

* sfu: don't count opus useinbandfec=0 as FEC in the down track

DownTrack decides whether opus FEC is on by looking for "fec" anywhere
in the fmtp line, both on Bind and when the upstream codec changes.
"useinbandfec=0" contains "fec", so a publisher that turns FEC off is
scored as if FEC were on. The connection quality scorer then lowers the
packet loss weight for opus from 8.0 to 5.33, and a subscriber with 8%
audio loss is reported as GOOD instead of POOR. The receiver side
already checks for useinbandfec=1, so the publisher's and subscribers'
scores disagree about the same stream.

Add utils.IsOpusInbandFECEnabled, which reads the useinbandfec
parameter by key and treats only a value of 1 as on, and use it for
all three FEC checks in the down track. The RED path already looked for
useinbandfec=1 and only gains tolerance of spaces around the parameter.

* sfu: read FEC from the upstream codec when the down track's codec changes

handleUpstreamCodecChange took the FEC flag from the codec the subscriber
negotiated, while Bind takes it from the upstream codec. FEC describes
what the publisher encodes, so after RED falls back to opus, a publisher
with useinbandfec=1 and a subscriber with useinbandfec=0 was scored as
having no FEC, and the reverse as having FEC.

Decide FEC in one place from the upstream codecs, using the upstream opus
codec for RED, and use it in both Bind and the codec change path. Log the
flag on codec change as Bind already does.

* sfu: use the same useinbandfec=1 check as the RED path for FEC

Drop utils.IsOpusInbandFECEnabled and check FEC with
strings.Contains(strings.ToLower(fmtp), "useinbandfec=1"), as the RED
path already did.

* sfu: move the FEC check to utils and match useinbandfec with a regex

Move the check to utils.IsFECEnabled, as asked in review, and match
useinbandfec=1 as one fmtp parameter with a regex, ignoring case and
spaces around the parameter and the "=". A publisher that sends
"minptime=10; useinbandfec = 1" was scored as having no FEC. Move the
tests to utils/helpers_test.go.

pkg/sfu/downtrack.go

@@ -23,7 +23,6 @@ import (
 	"math"
 	"math/rand"
 	"slices"
-	"strings"
 	"sync"
 	"time"
 
@@ -511,12 +510,10 @@ func (d *DownTrack) Bind(t webrtc.TrackLocalContext) (webrtc.RTPCodecParameters,
 			return
 		}
 
-		isFECEnabled := false
+		isFECEnabled := utils.IsFECEnabled(matchedUpstreamCodec, d.upstreamCodecs)
 		if mime.IsMimeTypeStringRED(matchedUpstreamCodec.MimeType) {
 			d.isRED = true
 			for _, c := range d.upstreamCodecs {
-				isFECEnabled = strings.Contains(strings.ToLower(c.SDPFmtpLine), "useinbandfec=1")
-
 				// assume upstream primary codec is opus since we only support it for audio now
 				if mime.IsMimeTypeStringOpus(c.MimeType) {
 					d.upstreamPrimaryPT = uint8(c.PayloadType)
@@ -539,8 +536,6 @@ func (d *DownTrack) Bind(t webrtc.TrackLocalContext) (webrtc.RTPCodecParameters,
 				)
 			}
 			d.primaryPT = uint8(primaryPT)
-		} else if mime.IsMimeTypeStringAudio(matchedUpstreamCodec.MimeType) {
-			isFECEnabled = strings.Contains(strings.ToLower(matchedUpstreamCodec.SDPFmtpLine), "fec")
 		}
 
 		logFields := []any{
@@ -666,7 +661,7 @@ func (d *DownTrack) handleUpstreamCodecChange(mimeType string) {
 
 	oldPT, oldRtxPT, oldCodec := d.payloadType.Load(), d.payloadTypeRTX.Load(), d.codec.Load().(webrtc.RTPCodecCapability)
 
-	var codec webrtc.RTPCodecParameters
+	var codec, matchedUpstreamCodec webrtc.RTPCodecParameters
 	for _, c := range d.upstreamCodecs {
 		if !mime.IsMimeTypeStringEqual(c.MimeType, mimeType) {
 			continue
@@ -675,6 +670,7 @@ func (d *DownTrack) handleUpstreamCodecChange(mimeType string) {
 		matchCodec, err := utils.CodecParametersFuzzySearch(c, d.negotiatedCodecParameters)
 		if err == nil {
 			codec = matchCodec
+			matchedUpstreamCodec = c
 			break
 		}
 	}
@@ -698,7 +694,8 @@ func (d *DownTrack) handleUpstreamCodecChange(mimeType string) {
 	d.rtpStats.SetClockRate(codec.RTPCodecCapability.ClockRate)
 	d.rtpStatsRTX.SetClockRate(codec.RTPCodecCapability.ClockRate)
 
-	isFECEnabled := strings.Contains(strings.ToLower(codec.SDPFmtpLine), "fec")
+	// FEC is what the publisher encodes, so check the upstream codec, not the negotiated one
+	isFECEnabled := utils.IsFECEnabled(matchedUpstreamCodec, d.upstreamCodecs)
 	d.bindLock.Unlock()
 
 	d.params.Logger.Infow(
@@ -706,6 +703,7 @@ func (d *DownTrack) handleUpstreamCodecChange(mimeType string) {
 		"oldPT", oldPT, "newPT", d.payloadType.Load(),
 		"oldRTXPT", oldRtxPT, "newRTXPT", d.payloadTypeRTX.Load(),
 		"oldCodec", oldCodec, "newCodec", codec.RTPCodecCapability,
+		"isFECEnabled", isFECEnabled,
 	)
 
 	receiver := d.Receiver()

pkg/sfu/utils/helpers.go

@@ -17,6 +17,7 @@ package utils
 import (
 	"errors"
 	"fmt"
+	"regexp"
 	"strconv"
 	"strings"
 
@@ -66,6 +67,28 @@ func FindRTXPayloadType(needle webrtc.PayloadType, haystack []webrtc.RTPCodecPar
 	return webrtc.PayloadType(0)
 }
 
+// matches useinbandfec=1 as one fmtp parameter, ignoring case and spaces around the parameter and the "="
+var fecEnabledRegex = regexp.MustCompile(`(?i)(^|;)\s*useinbandfec\s*=\s*1\s*(;|$)`)
+
+// IsFECEnabled reports whether codec carries opus in-band FEC. For RED, it checks the opus codec in codecs.
+func IsFECEnabled(codec webrtc.RTPCodecParameters, codecs []webrtc.RTPCodecParameters) bool {
+	switch {
+	case mime.IsMimeTypeStringRED(codec.MimeType):
+		for _, c := range codecs {
+			if mime.IsMimeTypeStringOpus(c.MimeType) {
+				return fecEnabledRegex.MatchString(c.SDPFmtpLine)
+			}
+		}
+		return false
+
+	case mime.IsMimeTypeStringAudio(codec.MimeType):
+		return fecEnabledRegex.MatchString(codec.SDPFmtpLine)
+
+	default:
+		return false
+	}
+}
+
 // GetHeaderExtensionID returns the ID of a header extension, or 0 if not found
 func GetHeaderExtensionID(extensions []interceptor.RTPHeaderExtension, extension webrtc.RTPHeaderExtensionCapability) int {
 	for _, h := range extensions {

pkg/sfu/utils/helpers_test.go

@@ -53,3 +53,62 @@ func TestFindRTXPayloadType(t *testing.T) {
 		})
 	}
 }
+
+func TestIsFECEnabled(t *testing.T) {
+	codec := func(mimeType mime.MimeType, fmtp string) webrtc.RTPCodecParameters {
+		return webrtc.RTPCodecParameters{
+			RTPCodecCapability: webrtc.RTPCodecCapability{MimeType: mimeType.String(), SDPFmtpLine: fmtp},
+		}
+	}
+	opusOn := codec(mime.MimeTypeOpus, "minptime=10;useinbandfec=1")
+	opusOff := codec(mime.MimeTypeOpus, "minptime=10;useinbandfec=0")
+	red := codec(mime.MimeTypeRED, "111/111")
+	vp8 := codec(mime.MimeTypeVP8, "")
+
+	t.Run("fmtp", func(t *testing.T) {
+		tests := []struct {
+			fmtp     string
+			expected bool
+		}{
+			{"minptime=10;useinbandfec=1", true},
+			{"useinbandfec=1", true},
+			{"useinbandfec=1;stereo=1", true},
+			{"minptime=10; useinbandfec = 1", true},
+			{"minptime=10;  useinbandfec=1 ;stereo=1", true},
+			{"minptime=10;UseInbandFEC=1", true},
+			{"minptime=10;useinbandfec=0", false},
+			{"minptime=10;useinbandfec=10", false},
+			{"minptime=10;xuseinbandfec=1", false},
+			{"minptime=10", false},
+			{"", false},
+		}
+		for _, tc := range tests {
+			t.Run(tc.fmtp, func(t *testing.T) {
+				opus := codec(mime.MimeTypeOpus, tc.fmtp)
+				require.Equal(t, tc.expected, IsFECEnabled(opus, []webrtc.RTPCodecParameters{opus}))
+			})
+		}
+	})
+
+	t.Run("codecs", func(t *testing.T) {
+		tests := []struct {
+			name     string
+			codec    webrtc.RTPCodecParameters
+			codecs   []webrtc.RTPCodecParameters
+			expected bool
+		}{
+			{"opus with FEC", opusOn, []webrtc.RTPCodecParameters{opusOn}, true},
+			{"opus without FEC", opusOff, []webrtc.RTPCodecParameters{opusOff}, false},
+			{"RED over opus with FEC", red, []webrtc.RTPCodecParameters{red, opusOn}, true},
+			{"RED over opus without FEC", red, []webrtc.RTPCodecParameters{red, opusOff}, false},
+			{"RED without an opus codec", red, []webrtc.RTPCodecParameters{red}, false},
+			{"video", vp8, []webrtc.RTPCodecParameters{vp8}, false},
+			{"no codec", webrtc.RTPCodecParameters{}, []webrtc.RTPCodecParameters{opusOn}, false},
+		}
+		for _, tc := range tests {
+			t.Run(tc.name, func(t *testing.T) {
+				require.Equal(t, tc.expected, IsFECEnabled(tc.codec, tc.codecs))
+			})
+		}
+	})
+}