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)

41 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

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)
Component: Untriaged → WebRTC: Signaling
Product: Firefox → Core
Can you include the offer and answer sdps?  they should be easily available from about:webrtc.  Thanks
Flags: needinfo?(tommcguinness)
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)
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
Attached file ff-log-PT109-PT124.txt
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.
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.
Bug 1221473: Do not treat answer as authoritative wrt payload types.
Assignee: nobody → docfaraday
https://reviewboard.mozilla.org/r/24529/#review22083

LGTM.

My only question is if the rest of our code can handle asymmetric payload types?
(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.
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.
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)
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 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+
Check back on try
Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday)
https://hg.mozilla.org/mozilla-central/rev/c733bc1211ac
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.