Fix matching codecs with different rate or channels #3021
Conversation
aler9
force-pushed
the
patch-codecs-match
branch
from
a488626 to
0e8d4ec
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3021 +/- ##
==========================================
+ Coverage 78.51% 78.58% +0.06%
==========================================
Files 89 89
Lines 11037 11104 +67
==========================================
+ Hits 8666 8726 +60
- Misses 1885 1891 +6
- Partials 486 487 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
aler9
force-pushed
the
patch-codecs-match
branch
7 times, most recently
from
54a37a9 to
eb71ae1
Compare
aler9
changed the title
aler9
force-pushed
the
patch-codecs-match
branch
from
eb71ae1 to
55b280c
Compare
aler9
changed the title
aler9
force-pushed
the
patch-codecs-match
branch
from
55b280c to
c10ed6e
Compare
Consider clock rate and channels when matching codecs. This allows to support codecs with the same MIME type but sample rate or channel count that might be different from the default ones, like PCMU, PCMA, LPCM and multiopus.
aler9
force-pushed
the
patch-codecs-match
branch
from
c10ed6e to
b31dde1
Compare
| @@ -24,6 +24,74 @@ func parseParameters(line string) map[string]string { | |||
| return parameters | |||
| } | |||
|
|
|||
| // ClockRateEqual checks whether two clock rates are equal. | |||
| func ClockRateEqual(mimeType string, valA, valB uint32) bool { | |||
There was a problem hiding this comment.
Correct me if I'm wrong, but I feel like this function ClockRateEqual might not belong in the fmtp library. Could this logic be more appropriate elsewhere? same for ChannelsEqual etc
| // Existing implementations often use VP8, H264 or Opus without setting clock rate or channels. | ||
| // Keep compatibility with these situations. | ||
| // It would be better to remove this exception in a future major release. | ||
| switch { |
There was a problem hiding this comment.
Do we need to handle this for other codecs like H.264 and VP9 as well?
Should we also match if either clock rate is 0 regardless of the codec (to maintain compatibility with cases where clock rate is not explicitly set)?
Edit: I think you forgot to add cases for them.
| @@ -36,7 +36,7 @@ func Test_RTPSender_ReplaceTrack(t *testing.T) { //nolint:cyclop | |||
| trackA, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeVP8}, "video", "pion") | |||
| assert.NoError(t, err) | |||
|
|
|||
| trackB, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeH264}, "video", "pion") | |||
| trackB, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeH264, ClockRate: 90000}, "video", "pion") | |||
There was a problem hiding this comment.
Can we keep this test as it was to ensure we fallback to matching without the clockRate for backward compatibility? Instead, we could add a new test
| @@ -400,7 +400,7 @@ func Test_TrackLocalStatic_Payloader(t *testing.T) { | |||
|
|
|||
| customPayloader := &customCodecPayloader{} | |||
| track, err := NewTrackLocalStaticSample( | |||
| RTPCodecCapability{MimeType: mimeTypeCustomCodec}, | |||
| RTPCodecCapability{MimeType: mimeTypeCustomCodec, ClockRate: 90000}, | |||
There was a problem hiding this comment.
Can we keep this test as it was to ensure we fallback to matching without the clockRate for backward compatibility? Instead, we could add a new test
Description
In pion/webrtc, currently codecs are matched regardless of the clock rate and the channel count, and this makes impossible to fully support codecs that might have a clock rate or channel count different than the default one, in particular LPCM, PCMU, PCMA and multiopus (the last one is a custom Opus variant present in the Chrome source code to support multichannel Opus).
For instance, let's suppose a peer (receiver) wants to receive an audio track encoded with LPCM, 48khz sample rate and 2 channels. This receiver doesn't know the audio codec yet, therefore it advertises all supported sample rates in the SDP:
The other peer (sender) receives the SDP, but since the clock rate and channel count are not taken into consideration when matching codecs, the sender codec
LPCM/48000/2is wrongly associated with the receiver codecLPCM/44100. The result is that the audio track cannot be decoded correctly from the receiver side.This patch fixes the issue and has been running smoothly in MediaMTX for almost a year.
Unfortunately, in lots of examples and tests, the clock rate is not present (and in fact they are producing horrible SDPs that contain
VP8/0instead ofVP8/90000and are incompatible with lots of servers) therefore this new check causes troubles in existing code. In order to maintain compatibility, i added exceptions for the VP8, H264 and VP9 codecs.In the future, it might be better to update examples (i can do it in a future patch) and remove the exception.