-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Remove RTX codec if no primary #3199
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.
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3199 +/- ##
==========================================
+ Coverage 78.00% 78.09% +0.08%
==========================================
Files 93 93
Lines 11778 11807 +29
==========================================
+ Hits 9188 9221 +33
+ Misses 2078 2074 -4
Partials 512 512
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
40b3807
to
adc5d7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!!
@cnderrauber Thank you for the review. I moved the function to filter out unattached RTX into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
One suggestion for making it more bullet proof, but I'll leave it up to you if want to add that or not.
rtpcodec.go
Outdated
@@ -155,6 +156,27 @@ func findRTXPayloadType(needle PayloadType, haystack []RTPCodecParameters) Paylo | |||
return PayloadType(0) | |||
} | |||
|
|||
// Given RTX CodecParameters find the primary CodecParameters if one exists. | |||
func findPrimaryPayloadTypeForRTX(needle RTPCodecParameters, haystack []RTPCodecParameters) PayloadType { | |||
parts := strings.Split(needle.SDPFmtpLine, "=") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth adding an additional sanity check here that the mime type for needle
is actually the RTX mime type - just in case someone calls this with a non RTX codec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about it too @nils-ohlmeier . Thank you.
I am doing it at call site. Because at call site, if the returned payload type is 0 (BTW, that is a valid payload type for PCMU which is a different pain 🙂 , but does not apply in this code), it means that there is no primary for the given RTX and that RTX is removed from list of codecs.
If this function is called with a non-RTX and if this code returns 0, it could remove the codec.
Maybe, this should return two values (PayloadType, error) and return an error if given needle
is not RTX? WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe (PayloadType, bool)
with the second return value indicating if needle is rtx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes PCMU is a problem. Maybe the right thing to do here is to rename you function to something like primaryPayloadTypeForRTXExists
and it just returns a bool?
I think that would avoid all (?) of these problems, or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh and BTW very cool PR. Thanks for fixing this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I think it will still need two return values. I will change it to
primaryPayloadTypeForRTXExists(needle, haystack) (isRTX bool, primaryExists bool)
Calls site can remove unattached RTX if isRTX, exists := primary...(); isRTX && !primaryExists {}
. Can then do the RTX check inside too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update PR with the change.
rtpcodec.go
Outdated
return PayloadType(0) | ||
} | ||
|
||
primaryPayloadType, err := strconv.Atoi(parts[1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this break if there is a rtx-time
? e.g apt=90;rtx-time=2000
maybe we can use the new internal fmtp parser https://github.com/pion/webrtc/blob/master/internal/fmtp/fmtp.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh, I did not know about the fmtp parser. Will check it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use fmtp parser.
I think that would be a nice thing to move it out of internal too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool thank you, yeah a good idea, We can do it in the next minor release :)
While adding transceivers from SetRemoteDescription, the filtered codecs could filter out the primart codec and leave the RTX codec in. Generating an answer with that fails `SetRemoteDescription` on remote peer due to an unrecognisable codec. Fix it by filtering out RTX is primary is not there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you :)
Description
While adding transceivers from SetRemoteDescription, the filtered codecs could filter out the primart codec and leave the RTX codec in. Generating an answer with that fails
SetRemoteDescription
on remote peer due to an unrecognisable codec. Fix it by filtering out RTX is primary is not there.