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

[itzmanish]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.52%. Comparing base (feeeebf) to head (626ad3e).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3016      +/-   ##
==========================================
+ Coverage   78.47%   78.52%   +0.05%     
==========================================
  Files          89       89              
  Lines       11037    11038       +1     
==========================================
+ Hits         8661     8668       +7     
+ Misses       1890     1885       -5     
+ Partials      486      485       -1     

Flag	Coverage Δ
go	80.08% <100.00%> (+0.06%)	⬆️
wasm	63.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[itzmanish]

While this PR fixes the issue of duplicate payload type codec entry on RegisterCodec, There is still one issue that this PR doesn't cover.

What if we have some registered codecs and the client sends the codecs with different payload types and one of the payload types the client is using clashes with our registered codecs, in that case, the client's codec won't be added to the negotiated codecs list. That's an issue because the server will now be missing that codec.

The flow -

Pion server codec list - []{..,PayloadType: 96, Mime: "video/VP8"..}
Browser sends codecs on sdp which triggers `updateFromRemoteDescription` - []{..., PayloadType: 96, Mime: "audio/OPUS"...}

This PR will make the media engine discard the opus codec. However, the issue remains even without this PR,

Pion server codec list - []{..,PayloadType: 96, Mime: "video/VP9", "profile-id=0"..}
Browser sends codecs on sdp which triggers `updateFromRemoteDescription` - []{..., PayloadType: 96, Mime: "video/VP9", "profile-id=1"...}

Again the client-given codec won't be added and most probably the browser will use "profile-id=0" one only (I am not sure about specs here, so please correct me here).

[JoeTurki]

@itzmanish Thank you so much, Will you be able to fix the lint issues (commit and code?).

What if we have some registered codecs and the client sends the codecs with different payload types and one of the payload types the client is using clashes with our registered codecs, in that case, the client's codec won't be added to the negotiated codecs list. That's an issue because the server will now be missing that codec.

For your use cause (SFU) you will need to manage separate payload type mappings for the send and receive directions in certain cases, as outlined in rfc3264

For recvonly RTP streams, the payload type numbers indicate the value
of the payload type field in RTP packets the offerer is expecting to
receive for that codec. For sendonly RTP streams, the payload type
numbers indicate the value of the payload type field in RTP packets
the offerer is planning to send for that codec. For sendrecv RTP
streams, the payload type numbers indicate the value of the payload
type field the offerer expects to receive, and would prefer to send.
However, for sendonly and sendrecv streams, the answer might indicate
different payload type numbers for the same codecs, in which case,
the offerer MUST send with the payload type numbers from the answer.
Different payload type numbers may be needed in each direction
because of interoperability concerns with H.323.

Worth mentioning rfc3551

This profile reserves payload type numbers in the range 96-127
exclusively for dynamic assignment. Applications SHOULD first use
values in this range for dynamic payload types. Those applications
which need to define more than 32 dynamic payload types MAY bind
codes below 96, in which case it is RECOMMENDED that unassigned
payload type numbers be used first. However, the statically assigned
payload types are default bindings and MAY be dynamically bound to
new encodings if needed. Redefining payload types below 96 may cause
incorrect operation if an attempt is made to join a session without
obtaining session description information that defines the dynamic
payload types.

Dynamic payload types SHOULD NOT be used without a well-defined
mechanism to indicate the mapping. Systems that expect to
interoperate with others operating under this profile SHOULD NOT make
their own assignments of proprietary encodings to particular, fixed
payload types.

Maybe we can make changes to accommodate this.
Pion supports dynamic payload types.

[itzmanish]

Will you be able to fix the lint issues (commit and code?).

Maybe need to re-trigger the commit linter to run. I have updated the PR title and last commit message to pass, but it's not getting triggered for newer commits. @JoeTurki could you please re-trigger the gh action for lint metadata?

[itzmanish]

However, for sendonly and sendrecv streams, the answer might indicate
different payload type numbers for the same codecs, in which case,
the offerer MUST send with the payload type numbers from the answer.

This line. let's say, the server is an offerer and generates SDP with PT 96, mime video/VP8 and PT 97, mime video/VP9. Could it be possible for the client to generate and answer SDP with PT 96, mime video/VP9 and PT 97, mime video/VP8? If yes, according to how we update negotiatedCodecs, it won't get updated.
But the spec says the offerer MUST send with the payload type numbers from the answer, With what PT number the server will send the packet?

[JoeTurki]

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.

[itzmanish]

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.

[JoeTurki]

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

[itzmanish]

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.

[itzmanish]

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?

[JoeTurki]

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.

[itzmanish]

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.

[JoeTurki]

Maybe need to re-trigger the commit linter to run. I have updated the PR title and last commit message to pass, but it's not getting triggered for newer commits. @JoeTurki could you please re-trigger the gh action for lint metadata?

PR title doesn't affect it, You'll need to reword your old commits, but since we only allow for one commit per PR it's better if you squash your commits into a single one and fix the lint issues :)

Thanks :)

[JoeTurki]

This line. let's say, the server is an offerer and generates SDP with PT 96, mime video/VP8 and PT 97, mime video/VP9. Could it be possible for the client to generate and answer SDP with PT 96, mime video/VP9 and PT 97, mime video/VP8? If yes, according to how we update negotiatedCodecs, it won't get updated.
But the spec says the offerer MUST send with the payload type numbers from the answer, With what PT number the server will send the packet?

Is this what you mean?

webrtc/mediaengine_test.go

Line 98 in feeeebf

t.Run("Change Payload Type", func(t *testing.T) {

[itzmanish]

yes. I get it now. The negotiated codecs list always respects the answer's codec preferences. Thanks :)