Repository: pion/webrtc (go)

Issue:
RegisterCodec can register two different codecs with same payload type
<!--
  Come join our community on Slack https://pion.ly/slack for quicker support and more resources

  FAQ of common issues and questions https://github.com/pion/webrtc/wiki/FAQ

  Free book that covers general WebRTC https://webrtcforthecurious.com/

  Please answer these questions before submitting your issue. Thanks!
-->

### Your environment.
  - Version: feeeebf251166faabdb00bd07b1ce2f6bd1064ad
  - Browser: Chrome/132.0.0.0
  - Other Information - https://gophers.slack.com/archives/CAK2124AG/p1737531701059119

### What did you do?
<!--
If possible, provide an example for reproducing the error.
A complete runnable program is good.
-->
I added two different codecs with the same payload type and upon negotiation with the browser the browser threw `Failed to parse SessionDescription. a=rtpmap:100 VP8/90000 Duplicate payload type with conflicting codec name or clock rate` error.

### What did you expect?
`RegisterCodec` should have returned an error and should not add codec with duplicate payload type.

### What happened?
The addCodec method is called from updateFromRemoteDescription and RegisterCodec as well. The issue is if there are different codecs with the same payload type, for eg- 100 VP8 (Firefox) and 100 VP9 (Chrome), getting registered. https://github.com/pion/webrtc/blob/feeeebf251166faabdb00bd07b1ce2f6bd1064ad/mediaengine.go#L249 This line will treat them as different codecs and add both of them in codecs slice.
When the getCodecsByKind method is called during SDP generate for either answer or offer, both codecs will be included in SDP. Chrome throws an error `Failed to parse SessionDescription. a=rtpmap:100 VP8/90000 Duplicate payload type with conflicting codec name or clock rate` when it gets the same payload type with different codec information.

Pull request:
Fix addCodec to return error if payload type exists in codecs list
#### Description
This PR adds changes to return an error if the codec already exists on the codecs list. This way now the implementing party should take care of duplicate codecs addition if the payload type is the same.
#### Reference issue
Fixes #3015


Patch:
diff --git a/mediaengine.go b/mediaengine.go
index 6a791dec..299726fa 100644
--- a/mediaengine.go
+++ b/mediaengine.go
@@ -7,6 +7,7 @@
 package webrtc
 
 import (
+	"errors"
 	"fmt"
 	"strconv"
 	"strings"
@@ -55,6 +56,9 @@ const (
 	MimeTypeFlexFEC = "video/flexfec"
 )
 
+// ErrCodecAlreadyRegistered indicates that a codec has already been registered for the same payload type.
+var ErrCodecAlreadyRegistered = fmt.Errorf("codec already registered for same payload type")
+
 type mediaEngineHeaderExtension struct {
 	uri              string
 	isAudio, isVideo bool
@@ -244,17 +248,20 @@ func (m *MediaEngine) RegisterDefaultCodecs() error {
 }
 
 // addCodec will append codec if it not exists.
-func (m *MediaEngine) addCodec(codecs []RTPCodecParameters, codec RTPCodecParameters) []RTPCodecParameters {
+func (m *MediaEngine) addCodec(codecs []RTPCodecParameters, codec RTPCodecParameters) ([]RTPCodecParameters, error) {
 	for _, c := range codecs {
-		if c.MimeType == codec.MimeType &&
-			fmtp.ClockRateEqual(c.MimeType, c.ClockRate, codec.ClockRate) &&
-			fmtp.ChannelsEqual(c.MimeType, c.Channels, codec.Channels) &&
-			c.PayloadType == codec.PayloadType {
-			return codecs
+		if c.PayloadType == codec.PayloadType {
+			if c.MimeType == codec.MimeType &&
+				fmtp.ClockRateEqual(c.MimeType, c.ClockRate, codec.ClockRate) &&
+				fmtp.ChannelsEqual(c.MimeType, c.Channels, codec.Channels) {
+				return codecs, nil
+			}
+
+			return codecs, ErrCodecAlreadyRegistered
 		}
 	}
 
-	return append(codecs, codec)
+	return append(codecs, codec), nil
 }
 
 // RegisterCodec adds codec to the MediaEngine
@@ -263,17 +270,18 @@ func (m *MediaEngine) RegisterCodec(codec RTPCodecParameters, typ RTPCodecType)
 	m.mu.Lock()
 	defer m.mu.Unlock()
 
+	var err error
 	codec.statsID = fmt.Sprintf("RTPCodec-%d", time.Now().UnixNano())
 	switch typ {
 	case RTPCodecTypeAudio:
-		m.audioCodecs = m.addCodec(m.audioCodecs, codec)
+		m.audioCodecs, err = m.addCodec(m.audioCodecs, codec)
 	case RTPCodecTypeVideo:
-		m.videoCodecs = m.addCodec(m.videoCodecs, codec)
+		m.videoCodecs, err = m.addCodec(m.videoCodecs, codec)
 	default:
 		return ErrUnknownType
 	}
 
-	return nil
+	return err
 }
 
 // RegisterHeaderExtension adds a header extension to the MediaEngine
@@ -577,14 +585,21 @@ func (m *MediaEngine) updateHeaderExtension(id int, extension string, typ RTPCod
 	return nil
 }
 
-func (m *MediaEngine) pushCodecs(codecs []RTPCodecParameters, typ RTPCodecType) {
+func (m *MediaEngine) pushCodecs(codecs []RTPCodecParameters, typ RTPCodecType) error {
+	var joinedErr error
 	for _, codec := range codecs {
+		var err error
 		if typ == RTPCodecTypeAudio {
-			m.negotiatedAudioCodecs = m.addCodec(m.negotiatedAudioCodecs, codec)
+			m.negotiatedAudioCodecs, err = m.addCodec(m.negotiatedAudioCodecs, codec)
 		} else if typ == RTPCodecTypeVideo {
-			m.negotiatedVideoCodecs = m.addCodec(m.negotiatedVideoCodecs, codec)
+			m.negotiatedVideoCodecs, err = m.addCodec(m.negotiatedVideoCodecs, codec)
+		}
+		if err != nil {
+			joinedErr = errors.Join(joinedErr, err)
 		}
 	}
+
+	return joinedErr
 }
 
 // Update the MediaEngine from a remote description.
@@ -645,13 +660,16 @@ func (m *MediaEngine) updateFromRemoteDescription(desc sdp.SessionDescription) e
 		// use exact matches when they exist, otherwise fall back to partial
 		switch {
 		case len(exactMatches) > 0:
-			m.pushCodecs(exactMatches, typ)
+			err = m.pushCodecs(exactMatches, typ)
 		case len(partialMatches) > 0:
-			m.pushCodecs(partialMatches, typ)
+			err = m.pushCodecs(partialMatches, typ)
 		default:
 			// no match, not negotiated
 			continue
 		}
+		if err != nil {
+			return err
+		}
 
 		if err := m.updateHeaderExtensionFromMediaSection(media); err != nil {
 			return err


Test patch:
diff --git a/mediaengine_test.go b/mediaengine_test.go
index 7f0ae392..15c27338 100644
--- a/mediaengine_test.go
+++ b/mediaengine_test.go
@@ -592,6 +592,26 @@ func TestMediaEngineDoubleRegister(t *testing.T) {
 	assert.Equal(t, len(mediaEngine.audioCodecs), 1)
 }
 
+// If a user attempts to register a codec with same payload but with different
+// codec we should just discard duplicate calls.
+func TestMediaEngineDoubleRegisterDifferentCodec(t *testing.T) {
+	mediaEngine := MediaEngine{}
+
+	assert.NoError(t, mediaEngine.RegisterCodec(
+		RTPCodecParameters{
+			RTPCodecCapability: RTPCodecCapability{MimeTypeG722, 8000, 0, "", nil},
+			PayloadType:        111,
+		}, RTPCodecTypeAudio))
+
+	assert.Error(t, ErrCodecAlreadyRegistered, mediaEngine.RegisterCodec(
+		RTPCodecParameters{
+			RTPCodecCapability: RTPCodecCapability{MimeTypeOpus, 48000, 0, "", nil},
+			PayloadType:        111,
+		}, RTPCodecTypeAudio))
+
+	assert.Equal(t, len(mediaEngine.audioCodecs), 1)
+}
+
 // The cloned MediaEngine instance should be able to update negotiated header extensions.
 func TestUpdateHeaderExtenstionToClonedMediaEngine(t *testing.T) {
 	src := MediaEngine{}
diff --git a/rtptransceiver_test.go b/rtptransceiver_test.go
index b7e1a722..298556a6 100644
--- a/rtptransceiver_test.go
+++ b/rtptransceiver_test.go
@@ -18,8 +18,8 @@ func Test_RTPTransceiver_SetCodecPreferences(t *testing.T) {
 	api := NewAPI(WithMediaEngine(mediaEngine))
 	assert.NoError(t, mediaEngine.RegisterDefaultCodecs())
 
-	mediaEngine.pushCodecs(mediaEngine.videoCodecs, RTPCodecTypeVideo)
-	mediaEngine.pushCodecs(mediaEngine.audioCodecs, RTPCodecTypeAudio)
+	assert.NoError(t, mediaEngine.pushCodecs(mediaEngine.videoCodecs, RTPCodecTypeVideo))
+	assert.NoError(t, mediaEngine.pushCodecs(mediaEngine.audioCodecs, RTPCodecTypeAudio))
 
 	tr := RTPTransceiver{kind: RTPCodecTypeVideo, api: api, codecs: mediaEngine.videoCodecs}
 	assert.EqualValues(t, mediaEngine.videoCodecs, tr.getCodecs())
diff --git a/sdp_test.go b/sdp_test.go
index 700f16fc..89caa6fd 100644
--- a/sdp_test.go
+++ b/sdp_test.go
@@ -683,8 +683,8 @@ func TestPopulateSDP(t *testing.T) { //nolint:cyclop,maintidx
 		me := &MediaEngine{}
 		assert.NoError(t, me.RegisterDefaultCodecs())
 		api := NewAPI(WithMediaEngine(me))
-		me.pushCodecs(me.videoCodecs, RTPCodecTypeVideo)
-		me.pushCodecs(me.audioCodecs, RTPCodecTypeAudio)
+		assert.NoError(t, me.pushCodecs(me.videoCodecs, RTPCodecTypeVideo))
+		assert.NoError(t, me.pushCodecs(me.audioCodecs, RTPCodecTypeAudio))
 
 		tr := &RTPTransceiver{kind: RTPCodecTypeVideo, api: api, codecs: me.videoCodecs}
 		tr.setDirection(RTPTransceiverDirectionRecvonly)