Closed Bug 818618 Opened 8 years ago Closed 5 years ago

Implement Opus stereo/mono codec configuration and SDP handling

Categories

(Core :: WebRTC: Signaling, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- wontfix
firefox45 --- wontfix
firefox46 --- fixed
firefox47 --- fixed
Blocking Flags:

People

(Reporter: abr, Assigned: bwc)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Currently, our webrtc signaling code contains a workaround for a bug in the WebRTC.org implementation wherein an attempt to configure two channels for the Opus codec fails (see http://code.google.com/p/webrtc/issues/detail?id=1013).

Once we update our WebRTC.org code to rev 3050 or later, the workaround should be pulled out.

At the moment, this workaround is in media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp in the function map_VCM_Media_Payload_type. Following the anticipated landing of the patch Bug 817065, this workaround will move to media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c, in the function "gsmsdp_negotiate_codec". Search for the string "FIXME".
Assignee: nobody → adam
Depends on: webrtc_updates
When the stereo handling is fixed in the webrtc.org code, we probably want to configure for stereo or mono based on the a=sprop-stereo and a=stereo values.
"derf: So we want to send with min(us:sprop-stereo,them:stereo), and receive with min(us:stereo,them:sprop-stereo)."
We also need to ensure that we send correct SDP attributes according to how we're configuring our engine.
Summary: Update codec configuration handling for Opus → Update codec configuration handling for stereo Opus
Whiteboard: [WebRTC], [blocking-webrtc-]
Note that the codec parameter changes need to be guided by the SDP handling cited in comment 1 and comment 2. I am currently intending this bug to handle both the SDP handling and the Opus configuration. The title has been updated accordingly.
Summary: Update codec configuration handling for stereo Opus → Implement Opus stereo/mono codec configuration and SDP handling
Attachment #702177 - Attachment is obsolete: true
Comment on attachment 712199 [details] [diff] [review]
adjust default opus frequency to 48000 in SDP

in-person r=derf
Attachment #712199 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3cc609f4744c
Whiteboard: [WebRTC], [blocking-webrtc-] → [WebRTC], [blocking-webrtc-][leave-open]
Priority: -- → P4
backlog: --- → webRTC+
Rank: 48
Whiteboard: [WebRTC], [blocking-webrtc-][leave-open] → [leave-open]
It seems very unlikely that I'll have an opportunity to work on WebRTC platform development for the foreseeable future. I'm unassigning this bug so that someone else might pick it up.
Assignee: adam → nobody
A lot of the implementation detail has changed here, but apparently this is still not working properly.
Assignee: nobody → docfaraday
Paul wrote on bug 1249098:
> What the team is telling me is that the existing clients cannot play stereo audio to the user.  Would > it be possible to force mono and then resolve the stereo/mono issue as another bug?

Can these existing clients play audio currently (with us encoding Opus as stereo)?  Except maybe in really unusual cases (if any), we're actually putting the same data into both channels.
Flags: needinfo?(paulej)
Making this a high priority (I'm assuming for now that we'll need this in Fx47), but I still want us to discuss with Cisco if this can be worked around in some other way.
Severity: trivial → major
Rank: 48 → 10
Priority: P4 → P1
QA Contact: jsmith
Comment on attachment 8724120 [details]
MozReview Request: Bug 818618: Honor (and emit) opus stereo fmtp param. r=jesup

https://reviewboard.mozilla.org/r/36877/#review33507

::: media/webrtc/signaling/src/sdp/SdpAttribute.h:1259
(Diff revision 1)
> -      os << "maxplaybackrate=" << maxplaybackrate;
> +      os << "maxplaybackrate=" << maxplaybackrate << "; "

I don't think it's normal to have spaces after commas in fmtp parameters, tough it may be syntactically valid.  (I saw one device a decade ago that insisted on a space, to the violation of the spec. We had to sniff the device manufacturer and change our params for that device to interop)

::: media/webrtc/signaling/test/sdp_unittests.cpp:1144
(Diff revision 1)
> -"a=fmtp:109 maxplaybackrate=32000" CRLF
> +"a=fmtp:109 maxplaybackrate=32000; stereo=1" CRLF

ditto for the tests

::: media/webrtc/signaling/test/sdp_unittests.cpp:1506
(Diff revision 1)
> -"a=fmtp:109 maxplaybackrate=32000" CRLF
> +"a=fmtp:109 maxplaybackrate=32000; stereo=1" CRLF

ditto
Attachment #8724120 - Flags: review?(rjesup) → review+
Randall, what I was told that some of our existing clients (the ones that are subject to bug 1249098 are also unable to play stereo.  If mono was the default, that would address the problem.  Newer endpoints can handle both.  The team is looking for a workaround, but confidence is low since we have little control over the legacy deployments.
Flags: needinfo?(paulej)
Comment on attachment 8724120 [details]
MozReview Request: Bug 818618: Honor (and emit) opus stereo fmtp param. r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36877/diff/1-2/
Attachment #8724120 - Attachment description: MozReview Request: Bug 818618: Honor (and emit) opus stereo fmtp param. r?jesup → MozReview Request: Bug 818618: Honor (and emit) opus stereo fmtp param. r=jesup
Try the binaries here and see if they work for you:

http://archive.mozilla.org/pub/firefox/try-builds/bcampen@mozilla.com-f99261d58e6a001a81d308f1c3bc14ff4acf9acd/
Flags: needinfo?(rdaware)
Flags: needinfo?(paulej)
(In reply to Byron Campen [:bwc] from comment #19)
> Try the binaries here and see if they work for you:
> 
> http://archive.mozilla.org/pub/firefox/try-builds/bcampen@mozilla.com-
> f99261d58e6a001a81d308f1c3bc14ff4acf9acd/

Hi Byron,

Thank you for the image. I verified this binary. It looks good now. Also, wanted to make sure if there is UT added for this bug. Thanks again.
Flags: needinfo?(rdaware)
(In reply to rdaware from comment #20)
> (In reply to Byron Campen [:bwc] from comment #19)
> > Try the binaries here and see if they work for you:
> > 
> > http://archive.mozilla.org/pub/firefox/try-builds/bcampen@mozilla.com-
> > f99261d58e6a001a81d308f1c3bc14ff4acf9acd/
> 
> Hi Byron,
> 
> Thank you for the image. I verified this binary. It looks good now. Also,
> wanted to make sure if there is UT added for this bug. Thanks again.

   The signaling/negotiation part is unit-tested, but our integration tests do not have the means of covering something like this right now.
Thanks for getting this tested so quickly!
Flags: needinfo?(paulej)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
Target Milestone: --- → mozilla47
Depends on: 1249098
No longer depends on: webrtc_updates
Comment on attachment 8724120 [details]
MozReview Request: Bug 818618: Honor (and emit) opus stereo fmtp param. r=jesup

Approval Request Comment
[Feature/regressing bug #]:

   Bug 953265 (sort of)

[User impact if declined]:

   Wasted bandwidth when interoping with endpoints that cannot make use of stereo opus, or even failure to interop with endpoints that cannot handle stereo opus.

[Describe test coverage new/current, TreeHerder]:

   There are some new unit-tests that cover the signaling aspects.

[Risks and why]: 

   Very low, not many changes were required here.

[String/UUID change made/needed]:

   None.
Attachment #8724120 - Flags: approval-mozilla-aurora?
Comment on attachment 8724120 [details]
MozReview Request: Bug 818618: Honor (and emit) opus stereo fmtp param. r=jesup

Approved, thanks for the extra tests, let's fix the wasted bandwidth problem
Attachment #8724120 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hitting conflicts on this, though that might be because I can't uplift bug 1249098 due to conflicts in that patch.
Flags: needinfo?(docfaraday)
This applies cleanly on the aurora backport of bug 1249098
Flags: needinfo?(docfaraday)
Is this actually available in FF46? I am trying to set audio to stereo, but still get a mono result. I also disabled echo cancellation to get rid of AEC, but this did not help either. I'm using these instructions as a reference how to enable stereo in WebRTC:

https://www.webrtcexample.com/blog/?go=all/how-to-support-stereo-in-a-webrtc-application/
(In reply to Tom Brückner from comment #30)
> Is this actually available in FF46? I am trying to set audio to stereo, but
> still get a mono result. I also disabled echo cancellation to get rid of
> AEC, but this did not help either. I'm using these instructions as a
> reference how to enable stereo in WebRTC:
> 
> https://www.webrtcexample.com/blog/?go=all/how-to-support-stereo-in-a-webrtc-
> application/

   Can you attach the about:webrtc for your case?
Sure, thanks for the super-fast reply. I added the aboutWebrtc.html for your reference. I'm using Chrome as a peer for Firefox, and I am transmitting two streams in two separate peer connections:

1. The video stream in the main PC
2. The audio stream in a secondary stream

Reason for this setup is that I am building an app for musicians where they can transmit additional audio streams along with the primary (chat) stream.

If I start the secondary stream in Chrome, FF receives it as stereo correctly and I can hear it in stereo (this worked even without patching the SDP manually). But if I try it the opposite around (i.e. start the stream in FF), Chrome receives it in mono (at least I can hear it only in mono). It's a stream that I feed from Ableton Live into a virtual audio device on my Mac, to be able to route it.
> in a secondary stream

should read: secondary PC
There's a stereo=0 in the remote SDP; since the stereo param describes what you're willing to receive, we send mono.
Ok, thanks! So I am trying to detect patch the answer SDP on Chrome side as well. I expected that this would be done automatically, since Chrome supports stereo.

Do you happen to know what might trigger Chrome to neglect the stereo request from FF and patch it down to mono? Is this "normal" behaviour?
Are you asking why Chrome uses stereo=0 when Firefox used stereo=1?

When firefox puts stereo=1 in its SDP, all that means is that firefox would like to receive stereo. It says nothing about what should happen in the other direction, and so it does not have anything to do with whether chrome also uses stereo=1.
(In reply to Byron Campen [:bwc] (PTO Apr 15) from comment #37)
> Are you asking why Chrome uses stereo=0 when Firefox used stereo=1?
> 
> When firefox puts stereo=1 in its SDP, all that means is that firefox would
> like to receive stereo. It says nothing about what should happen in the
> other direction, and so it does not have anything to do with whether chrome
> also uses stereo=1.

to rephrase byron: SDP (with a few exceptions) describes what you want/need to receive.  In this case, Chrome wants to receive mono, so we aren't going to send stereo if Chrome says stereo=0 in an offer (or an answer).
Thanks guys for these additional insights. I was indeed assuming the opposite, i.e. that the offer signals which resources are available and in which formats. So I guess it's the best way to use the signaling channel for this information, since I need the sender to determine what is actually transferred (the peer is just listening to the audio in my use-case).
I tried this out now. Both SDP are set to stereo now, but still the sound I receive in Chrome (sent from FF) is mono. And even worse, it sounds very very bad, like a poor quality phone line. It sounds as if something (echo cancellation?) is degrading the quality. If I try the same the other way around, i.e. by sending the audio from Chrome to FF, the quality I'm receiving in Firefox is very good.
This is just a guess, but the maxplaybackrate=0 in the remote SDP might be causing problems.
Yes, I noticed that as well and wondered whether this was causing problems. As I am using Muaz Khan's BandwidthHandler library to modify the SDP, I relied on his experience that this might be the right way to do it.
I set the maxplaybackrate to 48000 now, but this did not change anything. Still in mono and with bad quality.
I wonder what happens if you remove maxplaybackrate entirely. Maybe there's some problem with the patch on bug 1249098.
If I remove it completely, Firefox sets it to maxplaybackrate = 0 automatically. That was the reason why it was set this way initially.


In Chrome I send this as the answer SDP:

a=fmtp:109 minptime=10; useinbandfec=1; stereo=1; sprop-stereo=1

and Firefox interprets it like this (in about:webrtc):

a=fmtp:109 maxplaybackrate=0;stereo=1

Interestingly, the sprop-stereo=1 is missing as well.

I double-checked the SDP fingerprint, it's the very same.
How does it work when it is just firefox/firefox? Chrome/chrome? From what I can tell the audio quality is good with firefox/firefox (which will use stereo by default). It may be that Chrome just won't render stereo, regardless of what you put in the SDP.
I've tested it with two machines now, one Windows, one Mac. Communication direction was always from Mac (sending audio stream) to Windows (receiving audio stream), since it was easier to set-up a virtual audio device on the Mac to feed Ableton Live as an input device into WebRTC.

1. Chrome => Chrome

works perfectly, with top sound quality and rock-stable connection.

Mac offers SDP:
a=fmtp:111 minptime=10; useinbandfec=1; stereo=1; sprop-stereo=1

Windows answers with SDP:
a=fmtp:111 minptime=10; useinbandfec=1; stereo=1; sprop-stereo=1

2. Chrome => Firefox

Top sound quality and rock-stable connection. Yet, the left and right channels are swapped.

Mac offers SDP:
a=fmtp:111 minptime=10; useinbandfec=1; stereo=1; sprop-stereo=1
In about:webrtc on the Windows machine, this is shown as the external SDP: 
a=fmtp:111 maxplaybackrate=0;stereo=1

Windows answers with SDP:
a=fmtp:111 maxplaybackrate=48000; stereo=1; stereo=1; sprop-stereo=1
In about:webrtc, this is shown as the local SDP: 
a=fmtp:111 maxplaybackrate=48000;stereo=1

(The doubled stereo=1 is due to a bug in the BandwidthHandler library.)

3. Firefox => Chrome

Audio quality is not too bad (= no distortions, opposed to my tests Mac => Mac where the quality was inacceptable, as if some noise cancellation algorithm was working, and where drop-outs occurred from time to time, although both browsers were running on the same machine). However, the sound is mono only and seems to lack bass (which could be due to the mono quality).

Mac offers SDP:
a=fmtp:109 maxplaybackrate=48000;stereo=1; stereo=1; sprop-stereo=1

Windows answers with SDP:
a=fmtp:109 minptime=10; useinbandfec=1; stereo=1; sprop-stereo=1

4. Firefox => Firefox

Same as 3., mono.

Mac offers SDP:
a=fmtp:109 maxplaybackrate=48000;stereo=1; stereo=1; sprop-stereo=1
In about:webrtc on the Windows machine, this is shown as the external SDP: 
a=fmtp:109 maxplaybackrate=48000;stereo=1

Windows answers with SDP:
a=fmtp:109 maxplaybackrate=48000;stereo=1; stereo=1; sprop-stereo=1
In about:webrtc, this is shown as the local SDP: 
a=fmtp:109 maxplaybackrate=48000;stereo=1

So the mono conversion happens always if Firefox is the sending browser. If Chrome is sending, stereo works, but if Firefox is receiving the L/R channels are swapped.

If I can test anything else, just let me know.
Yes, the negotiation between Firefox and Chrome is not yet fully working...

If you wish, I can provide test accounts of sipcast.net to you, if you want to study the issues.
You need to log in before you can comment on or make changes to this bug.