Improve MediaEngine codec matching

Implement more sophisticated matching to prefer exact
matches over partial ones.

README.md

@@ -264,6 +264,7 @@ Check out the **[contributing wiki](https://github.com/pion/webrtc/wiki/Contribu
 * [Mats](https://github.com/Mindgamesnl)
 * [donotanswer](https://github.com/f-viktor)
 * [Reese](https://github.com/figadore)
+* [David Zhao](https://github.com/davidzhao)
 
 ### License
 MIT License - see [LICENSE](LICENSE) for full text

mediaengine.go

@@ -334,37 +334,25 @@ func (m *MediaEngine) collectStats(collector *statsReportCollector) {
 }
 
 // Look up a codec and enable if it exists
-func (m *MediaEngine) updateCodecParameters(remoteCodec RTPCodecParameters, typ RTPCodecType) error {
+func (m *MediaEngine) matchRemoteCodec(remoteCodec RTPCodecParameters, typ RTPCodecType) (codecMatchType, error) {
 	codecs := m.videoCodecs
 	if typ == RTPCodecTypeAudio {
 		codecs = m.audioCodecs
 	}
 
-	pushCodec := func(codec RTPCodecParameters) error {
-		if typ == RTPCodecTypeAudio {
-			m.negotiatedAudioCodecs = m.addCodec(m.negotiatedAudioCodecs, codec)
-		} else if typ == RTPCodecTypeVideo {
-			m.negotiatedVideoCodecs = m.addCodec(m.negotiatedVideoCodecs, codec)
-		}
-		return nil
-	}
-
 	if strings.HasPrefix(remoteCodec.RTPCodecCapability.SDPFmtpLine, "apt=") {
 		payloadType, err := strconv.Atoi(strings.TrimPrefix(remoteCodec.RTPCodecCapability.SDPFmtpLine, "apt="))
 		if err != nil {
-			return err
+			return codecMatchNone, err
 		}
 
 		if _, _, err = m.getCodecByPayload(PayloadType(payloadType)); err != nil {
-			return nil // not an error, we just ignore this codec we don't support
+			return codecMatchNone, nil // not an error, we just ignore this codec we don't support
 		}
 	}
 
-	if _, err := codecParametersFuzzySearch(remoteCodec, codecs); err == nil {
-		return pushCodec(remoteCodec)
-	}
-
-	return nil
+	_, matchType := codecParametersFuzzySearch(remoteCodec, codecs)
+	return matchType, nil
 }
 
 // Look up a header extension and enable if it exists
@@ -393,6 +381,16 @@ func (m *MediaEngine) updateHeaderExtension(id int, extension string, typ RTPCod
 	return nil
 }
 
+func (m *MediaEngine) pushCodecs(codecs []RTPCodecParameters, typ RTPCodecType) {
+	for _, codec := range codecs {
+		if typ == RTPCodecTypeAudio {
+			m.negotiatedAudioCodecs = m.addCodec(m.negotiatedAudioCodecs, codec)
+		} else if typ == RTPCodecTypeVideo {
+			m.negotiatedVideoCodecs = m.addCodec(m.negotiatedVideoCodecs, codec)
+		}
+	}
+}
+
 // Update the MediaEngine from a remote description
 func (m *MediaEngine) updateFromRemoteDescription(desc sdp.SessionDescription) error {
 	for _, media := range desc.MediaDescriptions {
@@ -413,10 +411,31 @@ func (m *MediaEngine) updateFromRemoteDescription(desc sdp.SessionDescription) e
 			return err
 		}
 
+		exactMatches := make([]RTPCodecParameters, 0, len(codecs))
+		partialMatches := make([]RTPCodecParameters, 0, len(codecs))
+
 		for _, codec := range codecs {
-			if err = m.updateCodecParameters(codec, typ); err != nil {
-				return err
+			matchType, mErr := m.matchRemoteCodec(codec, typ)
+			if mErr != nil {
+				return mErr
 			}
+
+			if matchType == codecMatchExact {
+				exactMatches = append(exactMatches, codec)
+			} else if matchType == codecMatchPartial {
+				partialMatches = append(partialMatches, codec)
+			}
+		}
+
+		// use exact matches when they exist, otherwise fall back to partial
+		switch {
+		case len(exactMatches) > 0:
+			m.pushCodecs(exactMatches, typ)
+		case len(partialMatches) > 0:
+			m.pushCodecs(partialMatches, typ)
+		default:
+			// no match, not negotiated
+			continue
 		}
 
 		extensions, err := rtpExtensionsFromMediaDescription(media)

mediaengine_test.go

@@ -189,6 +189,97 @@ a=rtpmap:111 opus/48000/2
 		assert.True(t, midAudioEnabled)
 		assert.False(t, midVideoEnabled)
 	})
+
+	t.Run("Prefers exact codec matches", func(t *testing.T) {
+		const profileLevels = `v=0
+o=- 4596489990601351948 2 IN IP4 127.0.0.1
+s=-
+t=0 0
+m=video 60323 UDP/TLS/RTP/SAVPF 96 98
+a=rtpmap:96 H264/90000
+a=fmtp:96 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=640c1f
+a=rtpmap:98 H264/90000
+a=fmtp:98 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f
+`
+		m := MediaEngine{}
+		assert.NoError(t, m.RegisterCodec(RTPCodecParameters{
+			RTPCodecCapability: RTPCodecCapability{MimeTypeH264, 90000, 0, "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f", nil},
+			PayloadType:        127,
+		}, RTPCodecTypeVideo))
+		assert.NoError(t, m.updateFromRemoteDescription(mustParse(profileLevels)))
+
+		assert.True(t, m.negotiatedVideo)
+		assert.False(t, m.negotiatedAudio)
+
+		supportedH264, _, err := m.getCodecByPayload(98)
+		assert.NoError(t, err)
+		assert.Equal(t, supportedH264.MimeType, MimeTypeH264)
+
+		_, _, err = m.getCodecByPayload(96)
+		assert.Error(t, err)
+	})
+
+	t.Run("Does not match when fmtpline is set and does not match", func(t *testing.T) {
+		const profileLevels = `v=0
+o=- 4596489990601351948 2 IN IP4 127.0.0.1
+s=-
+t=0 0
+m=video 60323 UDP/TLS/RTP/SAVPF 96 98
+a=rtpmap:96 H264/90000
+a=fmtp:96 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=640c1f
+`
+		m := MediaEngine{}
+		assert.NoError(t, m.RegisterCodec(RTPCodecParameters{
+			RTPCodecCapability: RTPCodecCapability{MimeTypeH264, 90000, 0, "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f", nil},
+			PayloadType:        127,
+		}, RTPCodecTypeVideo))
+		assert.Error(t, m.updateFromRemoteDescription(mustParse(profileLevels)))
+
+		_, _, err := m.getCodecByPayload(96)
+		assert.Error(t, err)
+	})
+
+	t.Run("Matches when fmtpline is not set in offer, but exists in mediaengine", func(t *testing.T) {
+		const profileLevels = `v=0
+o=- 4596489990601351948 2 IN IP4 127.0.0.1
+s=-
+t=0 0
+m=video 60323 UDP/TLS/RTP/SAVPF 96
+a=rtpmap:96 VP9/90000
+`
+		m := MediaEngine{}
+		assert.NoError(t, m.RegisterCodec(RTPCodecParameters{
+			RTPCodecCapability: RTPCodecCapability{MimeTypeVP9, 90000, 0, "profile-id=0", nil},
+			PayloadType:        98,
+		}, RTPCodecTypeVideo))
+		assert.NoError(t, m.updateFromRemoteDescription(mustParse(profileLevels)))
+
+		assert.True(t, m.negotiatedVideo)
+
+		_, _, err := m.getCodecByPayload(96)
+		assert.NoError(t, err)
+	})
+
+	t.Run("Matches when fmtpline exists in neither", func(t *testing.T) {
+		const profileLevels = `v=0
+o=- 4596489990601351948 2 IN IP4 127.0.0.1
+s=-
+t=0 0
+m=video 60323 UDP/TLS/RTP/SAVPF 96
+a=rtpmap:96 VP8/90000
+`
+		m := MediaEngine{}
+		assert.NoError(t, m.RegisterCodec(RTPCodecParameters{
+			RTPCodecCapability: RTPCodecCapability{MimeTypeVP8, 90000, 0, "", nil},
+			PayloadType:        96,
+		}, RTPCodecTypeVideo))
+		assert.NoError(t, m.updateFromRemoteDescription(mustParse(profileLevels)))
+
+		assert.True(t, m.negotiatedVideo)
+
+		_, _, err := m.getCodecByPayload(96)
+		assert.NoError(t, err)
+	})
 }
 
 func TestMediaEngineHeaderExtensionDirection(t *testing.T) {

rtpcodec.go

@@ -85,23 +85,34 @@ type RTPParameters struct {
 	Codecs           []RTPCodecParameters
 }
 
+type codecMatchType int
+
+const (
+	codecMatchNone    codecMatchType = 0
+	codecMatchPartial codecMatchType = 1
+	codecMatchExact   codecMatchType = 2
+)
+
 // Do a fuzzy find for a codec in the list of codecs
 // Used for lookup up a codec in an existing list to find a match
-func codecParametersFuzzySearch(needle RTPCodecParameters, haystack []RTPCodecParameters) (RTPCodecParameters, error) {
+// Returns codecMatchExact, codecMatchPartial, or codecMatchNone
+func codecParametersFuzzySearch(needle RTPCodecParameters, haystack []RTPCodecParameters) (RTPCodecParameters, codecMatchType) {
 	// First attempt to match on MimeType + SDPFmtpLine
+	// Exact matches means fmtp line cannot be empty
 	for _, c := range haystack {
 		if strings.EqualFold(c.RTPCodecCapability.MimeType, needle.RTPCodecCapability.MimeType) &&
 			c.RTPCodecCapability.SDPFmtpLine == needle.RTPCodecCapability.SDPFmtpLine {
-			return c, nil
+			return c, codecMatchExact
 		}
 	}
 
-	// Fallback to just MimeType
+	// Fallback to just MimeType if either haystack or needle does not have fmtpline set
 	for _, c := range haystack {
-		if strings.EqualFold(c.RTPCodecCapability.MimeType, needle.RTPCodecCapability.MimeType) {
-			return c, nil
+		if strings.EqualFold(c.RTPCodecCapability.MimeType, needle.RTPCodecCapability.MimeType) &&
+			(c.RTPCodecCapability.SDPFmtpLine == "" || needle.RTPCodecCapability.SDPFmtpLine == "") {
+			return c, codecMatchPartial
 		}
 	}
 
-	return RTPCodecParameters{}, ErrCodecNotFound
+	return RTPCodecParameters{}, codecMatchNone
 }

track_local_static.go

@@ -48,7 +48,7 @@ func (s *TrackLocalStaticRTP) Bind(t TrackLocalContext) (RTPCodecParameters, err
 	defer s.mu.Unlock()
 
 	parameters := RTPCodecParameters{RTPCodecCapability: s.codec}
-	if codec, err := codecParametersFuzzySearch(parameters, t.CodecParameters()); err == nil {
+	if codec, matchType := codecParametersFuzzySearch(parameters, t.CodecParameters()); matchType != codecMatchNone {
 		s.bindings = append(s.bindings, trackBinding{
 			ssrc:        t.SSRC(),
 			payloadType: codec.PayloadType,

track_local_static_test.go

@@ -35,7 +35,7 @@ func Test_TrackLocalStatic_NoCodecIntersection(t *testing.T) {
 		_, err = pc.AddTrack(track)
 		assert.NoError(t, err)
 
-		assert.True(t, errors.Is(signalPair(pc, noCodecPC), ErrUnsupportedCodec))
+		assert.ErrorIs(t, signalPair(pc, noCodecPC), ErrUnsupportedCodec)
 
 		closePairNow(t, noCodecPC, pc)
 	})