Add support for the WebRTC transport-cc extension for audio m= sections
Categories
(Core :: WebRTC: Networking, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox80 | --- | affected |
People
(Reporter: juberti, Unassigned)
References
Details
Attachments
(2 obsolete files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/84.0.4147.89 Safari/537.36
Steps to reproduce:
As noted in https://bugzilla.mozilla.org/show_bug.cgi?id=1606823, while transport-cc has been enabled for video m= sections, it is not enabled for audio m= sections.
This can be reproduced by going to https://webrtc.github.io/samples/src/content/peerconnection/create-offer/ in Nightly, which will generate the results below.
Actual results:
If you check only the 'offer to receive audio' box, you will see that the audio m= section does not contain the necessary lines, namely:
a=extmap:3 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a=rtcp-fb:111 transport-cc
However, if you check the 'offer to receive video' box, then the video m= section will contain the transport-cc extmap and rtcp-fb lines., but only the video section.
Example of audio m= section without transport-cc:
v=0
o=mozilla...THIS_IS_SDPARTA-80.0a1 3559983792781081640 0 IN IP4 0.0.0.0
s=-
t=0 0
a=fingerprint:sha-256 B9:08:17:04:F1:C9:63:82:23:8B:BA:C2:47:48:5D:6D:EB:38:7D:01:CA:07:E9:E8:02:6A:AC:DA:1E:0D:CF:A6
a=group:BUNDLE 0
a=ice-options:trickle
a=msid-semantic:WMS *
m=audio 9 UDP/TLS/RTP/SAVPF 109 9 0 8 101
c=IN IP4 0.0.0.0
a=sendrecv
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=extmap:2/recvonly urn:ietf:params:rtp-hdrext:csrc-audio-level
a=extmap:3 urn:ietf:params:rtp-hdrext:sdes:mid
a=fmtp:109 maxplaybackrate=48000;stereo=1;useinbandfec=1
a=fmtp:101 0-15
a=ice-pwd:0330d975318e425f1555d8d9b27b7faa
a=ice-ufrag:3eec7182
a=mid:0
a=msid:{a4ab4c22-bb6f-274c-bc13-6d7720920bbe} {86929781-9531-d643-b57e-5ae2ad237d25}
a=rtcp-mux
a=rtpmap:109 opus/48000/2
a=rtpmap:9 G722/8000/1
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:101 telephone-event/8000/1
a=setup:actpass
a=ssrc:4148384606 cname:{73afa26c-80c8-874f-9dae-3a280a6209a2}
Expected results:
Both audio and video m= sections should contain transport-cc extmap and rtcp-fb lines for the appropriate payload types. Both Chrome and Safari generate the expected results on this page.
Comment 2•4 years ago
|
||
Byron, was there any reason we didn't support this the first time around?
Comment 3•4 years ago
|
||
So this was initially implemented by a community contributor to solve video problems, it seems. It might be easy to switch it on for audio.
Updated•4 years ago
|
Updated•4 years ago
|
Reporter | ||
Comment 4•4 years ago
|
||
Adding some folks from the original bug.
Reporter | ||
Comment 5•4 years ago
|
||
Looks like it should be fairly straightforward to make a patch similar to https://phabricator.services.mozilla.com/D68734 that adds support for transport-cc to AudioCodecConfig and JsepAudioCodecDescription.
Comment 6•4 years ago
|
||
Hi Justin, it looks like this is only being enabled for opus. We'll do the same in Firefox, but I was wondering if there was a reason for this? Thanks!
Reporter | ||
Comment 7•4 years ago
|
||
For libwebrtc, the intention was to only enable this for codecs that support VBR. That said, that limitation seems somewhat artificial (as one might want to get feedback messages regardless of whether bitrate is being adapted), and so a more general approach for Firefox might make sense.
My immediate need though is just for opus.
Comment 8•4 years ago
|
||
(In reply to Justin Uberti from comment #7)
For libwebrtc, the intention was to only enable this for codecs that support VBR. That said, that limitation seems somewhat artificial (as one might want to get feedback messages regardless of whether bitrate is being adapted), and so a more general approach for Firefox might make sense.
My immediate need though is just for opus.
Thanks, I don't have a strong opinion on this, so I'll enable it for every codec and see what my reviewer thinks.
Comment 9•4 years ago
|
||
Comment 10•4 years ago
|
||
Depends on D85282
Comment 11•4 years ago
|
||
Unfortunately, I'm seeing deadlocks with this enabled. The PacerThread thread takes overhead_per_packet_lock_ at [1] and then waits for acm_crit_sect_ at [2]. Meanwhile, the AudioEncoderQue thread takes acm_crit_sect_ at [3] and waits for at overhead_per_packet_lock_ [4]. Not to be left out, the Socket thread is also blocked at [2], wanting to reconfigure the encoder based upon receiving RTCP at [5].
[1] https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/media/webrtc/trunk/webrtc/voice_engine/channel.cc#1841
[2] https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/audio_coding_module.cc#588
[3] https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/media/webrtc/trunk/webrtc/modules/audio_coding/acm2/audio_coding_module.cc#660
[4] https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/media/webrtc/trunk/webrtc/voice_engine/channel.cc#1841
[5] https://searchfox.org/mozilla-central/rev/cffd9b5302b6b6f51533d895a785b48ff418aec1/media/webrtc/trunk/webrtc/voice_engine/channel.cc#1371
Reporter | ||
Comment 12•4 years ago
|
||
Do you want to raise this on discuss-webrtc? I could also send this to the folks at Google who would know this code.
Comment 13•4 years ago
|
||
Because we run an older version of libwebrtc it's possible this has already been fixed upstream. It's also quite possible that this is showing up because the threading model in Firefox does not match what is expected by libwebrtc in this instance. I'll look in to this more on our side.
Comment 14•4 years ago
|
||
There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:dminor, could you have a look please?
For more information, please visit auto_nag documentation.
Comment 15•4 years ago
|
||
As mentioned above, the patches here will lead to deadlock if landed as is.
Comment 16•4 years ago
|
||
At this point, I think we'll wait until after the libwebrtc update lands to do anything more with this.
Comment 17•3 years ago
|
||
Renaming to clarify the bug is limited to audio (video appears to work).
Updated•3 years ago
|
Comment 18•3 years ago
|
||
Would involve a little refactoring inside the JsepCodecDescription stuff to allow JsepAudioCodecDescription to support rtcp-fb, tweaking the extmap setup in JsepSessionImpl, and removing the extmap filtering in the conduit code. It might just work without a hitch, but there may be difficulties with the libwebrtc integration.
Updated•2 years ago
|
Updated•2 years ago
|
Description
•