1654642 - Add support for the WebRTC transport-cc extension for audio m= sections

Status: NEW

(Core :: WebRTC: Networking, defect, P3)

Description

[Justin Uberti]

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 1

[Alex Chronopoulos [:achronop]]

Dan, can you please help me triage this one?

Comment 2

[Dan Minor [:dminor]]

Byron, was there any reason we didn't support this the first time around?

Comment 3

[Byron Campen [:bwc]]

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.

Comment 4

[Justin Uberti]

Adding some folks from the original bug.

Comment 5

[Justin Uberti]

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

[Dan Minor [:dminor]]

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!

Comment 7

[Justin Uberti]

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

[Dan Minor [:dminor]]

(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

[Dan Minor [:dminor]]

Attachment: Bug 1654642 - Enable transport-cc for audio codecs; r=bwc!

Comment 10

[Dan Minor [:dminor]]

Attachment: Bug 1654642 - Update tests for audio transport-cc; r=bwc!

Depends on D85282

Comment 11

[Dan Minor [:dminor]]

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

Comment 12

[Justin Uberti]

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

[Dan Minor [:dminor]]

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

[BugBot [:suhaib / :marco/ :calixte]]

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

[Dan Minor [:dminor]]

As mentioned above, the patches here will lead to deadlock if landed as is.

Comment 16

[Dan Minor [:dminor]]

At this point, I think we'll wait until after the libwebrtc update lands to do anything more with this.

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Renaming to clarify the bug is limited to audio (video appears to work).

Comment 18

[Byron Campen [:bwc]]

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.