Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix addCodec to return error if payload type exists in codecs list #3016

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix addCodec to return error if payload type exists in codecs list #3016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36f7b00
fix: return err if codec with same payload type exists in addCodec
itzmanish Jan 22, 2025
bf0168f
fix: lint issues
itzmanish Jan 22, 2025
626ad3e
Pass metadata lint
itzmanish Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 13 additions & 9 deletions mediaengine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,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
Expand Down Expand Up @@ -244,14 +247,14 @@ 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 && c.PayloadType == codec.PayloadType {
return codecs
if c.PayloadType == codec.PayloadType {
return codecs, ErrCodecAlreadyRegistered
}
}

return append(codecs, codec)
return append(codecs, codec), nil
}

// RegisterCodec adds codec to the MediaEngine
Expand All @@ -261,17 +264,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
Expand Down Expand Up @@ -573,9 +577,9 @@ func (m *MediaEngine) updateHeaderExtension(id int, extension string, typ RTPCod
func (m *MediaEngine) pushCodecs(codecs []RTPCodecParameters, typ RTPCodecType) {
for _, codec := range codecs {
if typ == RTPCodecTypeAudio {
m.negotiatedAudioCodecs = m.addCodec(m.negotiatedAudioCodecs, codec)
m.negotiatedAudioCodecs, _ = m.addCodec(m.negotiatedAudioCodecs, codec)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider handling this error explicitly? If someone mistakenly changes matchRemoteCodec.

I think it might be better to scope the logic of your PR to specific cases, such as only handling it only for videoCodecs and audioCodecs (calls originating from RegisterCodec). This would make sure we’re not inadvertently affecting unrelated handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider handling this error explicitly?

Honestly, we don't have to handle the error here, because push codec only happens on either partial or full match. If it's a full match, the codec won't be pushed anyway, and if it's partial I don't think it will be possible for the client to send two different codecs with the same payload type on SDP. So error handling doesn't matter here.

Copy link
Member

@JoeTurki JoeTurki Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true, but my comment was more about if someone updates matchRemoteCodec in the future, or any side effects. Maybe we only introduce this change only to the public API RegisterCodec or what do you think?

I don't think it will be possible for the client to send two different codecs with the same payload type on SDP.

I think we should return an error but I think we should do that in another PR.

I'm not entirely sure about this PR, maybe we should wait for someone with better understanding of the codebase, But I agree that RegisterCodec should return an error for dynamic PT reuse.

@Sean-Der

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we only introduce this change only to the public API RegisterCodec or what do you think?

For my use case only doing this and having publically accessible GetCodecsByKind is fine (separate PR). It's just currently the least possible change path where without doing another loop to find if the payload type already exists or not, we can achieve the error returning in case the codec is available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should return an error but I think we should do that in another PR.

Is there any possible scenario you can think of, where the client sends two different codecs with the same payload type?

Copy link
Member

@JoeTurki JoeTurki Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just currently the least possible change path where without doing another loop to find if the payload type already exists or not, we can achieve the error returning in case the codec is available.

You can just pass a Boolean (unique) to addCodec and conditionally return an error if it's true or refactor it to two functions one for internals, and one for matchedcodecs. Or any other option, The implementation doesn't matter, we can fix the code later; What matters is the behavior.

Is there any possible scenario you can think of, where the client sends two different codecs with the same payload type?

Yes, A misconfigured client, You found this "bug" because Chrome returned an error when you re-used the PT for different codecs. And I think we should return an error, to make sure we don't have an unexpected behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, A misconfigured client, You found this "bug" because Chrome returned an error when you re-used the PT for different codecs. And I think we should return an error, to make sure we don't have an unexpected behavior.

I totally agree. I can make those changes in the same PR or wait for @Sean-Der's input. It makes sense to handle this error on updateFromRemoteDescription.

} else if typ == RTPCodecTypeVideo {
m.negotiatedVideoCodecs = m.addCodec(m.negotiatedVideoCodecs, codec)
m.negotiatedVideoCodecs, _ = m.addCodec(m.negotiatedVideoCodecs, codec)
itzmanish marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
20 changes: 20 additions & 0 deletions mediaengine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,27 @@ func TestMediaEngineDoubleRegister(t *testing.T) {
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)
}

// 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,
Expand Down
Loading