Closed
Bug 1221473
Opened 9 years ago
Closed 8 years ago
No audio on browser side when Opus payload types in offer and answer do not match
Categories
(Core :: WebRTC: Signaling, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: tommcguinness, Assigned: bwc)
Details
Attachments
(3 files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36 Steps to reproduce: Make an outgoing WebRTC call between Firefox and another SIP client where the callee answers the FF offer with a payload type for Opus that is not the same as the original offer. In this case FF offers PT=109 for Opus Our client offers PT=124 for Opus in its answer Actual results: RTP flows from FF to our client with PT=124 => audio on our client is perfect RTP flows from our client to FF with PT=109 => audio on FF side is missing or sounds like weird radio static RTP flow was checked with Wireshark Expected results: Audio should be perfectly clear on both sides Making a call from Chrome using the exact same web client results in perfect bi-directional audio. If I force our client to use PT=109 for Opus in its answer then audio is perfect in both directions. This bug creates serious interop problems especially when calls are pushed to the web client via XMPP. In the push call flow both caller and caller take on the role of caller and the callee does not see the caller's offer until the SIP 200 OK (in SIP both sides send INVITE to a call control server that bridges the call segments)
Updated•9 years ago
|
Component: Untriaged → WebRTC: Signaling
Product: Firefox → Core
Comment 1•9 years ago
|
||
Can you include the offer and answer sdps? they should be easily available from about:webrtc. Thanks
Flags: needinfo?(tommcguinness)
Reporter | ||
Comment 2•9 years ago
|
||
There are three sets of SIP traces including SDP: - (FF-109-Maaii-124.txt) FF call to a iOS SIP client where SIP client using PT=124 for Opus. FF had no audio in this call. iOS SIP client could hear FF. - (FF-109-Maaii-109.txt) FF call to same client but this time iOS client uses PT=109 for Opus. Audio in both directions was perfect. - (FF-111-Chrome-111.txt) Chrome calls FF. PT for both sides was 111. Audio was perfect.
Flags: needinfo?(tommcguinness)
Comment 3•9 years ago
|
||
Thanks! one last thing that would help: If you could run Firefox with these environment vars: NSPR_LOG_DEBUG=signaling:4 NSPR_LOG_FILE=whatever_file_you_want and attach those logs to this bug. Thanks!
Status: UNCONFIRMED → NEW
backlog: --- → webrtc/webaudio+
Rank: 23
Ever confirmed: true
Priority: -- → P2
Reporter | ||
Comment 4•9 years ago
|
||
Here they are. Call from FF to Maaii (opus PT=124) Audio on FF side is missing. I am guessing that you are pushing the wrong payload type into VoECodec::SetRecPayloadType. It should be set to use PT=124. Let me know if you need any further traces.
Assignee | ||
Comment 5•9 years ago
|
||
It looks like there is a long-standing (dating back to the sipcc days) misunderstanding of how to handle asymmetric payload types in the code.
Assignee | ||
Comment 6•9 years ago
|
||
Bug 1221473: Do not treat answer as authoritative wrt payload types.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → docfaraday
Comment 7•9 years ago
|
||
https://reviewboard.mozilla.org/r/24529/#review22083 LGTM. My only question is if the rest of our code can handle asymmetric payload types?
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to Nils Ohlmeier [:drno] from comment #7) > https://reviewboard.mozilla.org/r/24529/#review22083 > > LGTM. > > My only question is if the rest of our code can handle asymmetric payload > types? Sadly, since we cannot make JsepCodecDescription answer with an asymmetric payload type, we cannot test this against ourselves. We might be able to teach the code to allow the payload type to be changed before SetLocal though.
Comment 9•9 years ago
|
||
We should add a unittest for this (followup bug); asymmetric payload types is definitely required to support (ditto for most other parameters modulo what offer-answer says to do with the types) - for most you must send what the other side told you to, and the offer/answer restrictions are on what's an acceptable answer (what can be downgraded or added in an answer, etc). Hopefully the rest of it we have right already.
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8684281 [details] MozReview Request: Bug 1221473: Do not treat answer as authoritative wrt payload types. r?drno Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24529/diff/1-2/
Attachment #8684281 -
Attachment description: MozReview Request: Bug 1221473: Do not treat answer as authoritative wrt payload types. → MozReview Request: Bug 1221473: Do not treat answer as authoritative wrt payload types. r?drno
Attachment #8684281 -
Flags: review?(drno)
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8684281 [details] MozReview Request: Bug 1221473: Do not treat answer as authoritative wrt payload types. r?drno Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24529/diff/2-3/
Comment 12•8 years ago
|
||
Comment on attachment 8684281 [details] MozReview Request: Bug 1221473: Do not treat answer as authoritative wrt payload types. r?drno https://reviewboard.mozilla.org/r/24529/#review34213 Looks like revision 3 is identical to the revision 1 which I r+ already :-) I think Randell is right we should at least have a follow up bug for testing asymmetric PT's (even if that bug will only contain discussion why we can't do it).
Attachment #8684281 -
Flags: review?(drno) → review+
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(docfaraday)
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c733bc1211ac
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•