1150609 - WebRTC Offer SDP should include tmmbr.

Status: RESOLVED FIXED

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

Description

[Ethan Hugg [:ehugg]]

According to the rtcweb-rtp-usage, supporting tmmbr is required:
https://tools.ietf.org/html/draft-ietf-rtcweb-rtp-usage-23#section-5.1.6

It appears that at least since the update to webrtc40 we honor incoming tmmbr requests.

What we don't do is put tmmbr in the offer so the other side will know we support it.

There's also the issue that we don't parse the tmmbr info from the remote SDP - that is bug 1097169.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: WebRTC SDP - add tmmbr to offer.

Comment 2

[Ethan Hugg [:ehugg]]

Attachment: WebRTC enable tmmbr

Comment 3

[Ethan Hugg [:ehugg]]

Attachment: WebRTC enable tmmbr unittest changes.

Comment 4

[Ethan Hugg [:ehugg]]

Try builds for testing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdf4cd48c674

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8588687 [details] [diff] [review]
WebRTC enable tmmbr

Review of attachment 8588687 [details] [diff] [review]:
-----------------------------------------------------------------

Just a question: (perhaps for byron) if we renegotiate, and the CCM/FB options change, do we re-init the media channel (and call ConfigureRecvMediaCodecs)?

Do we need to enable TMMBR on ConfigureSendMediaCodec as well?   Probably not... I presume if we receive a TMMBR we'll send TMMBN

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +864,5 @@
>    }
>    mUsingNackBasic = use_nack_basic;
>  
> +  if (use_tmmbr) {
> +    CSFLogDebug(logTag, "Enabling TMMBR (recv) for video stream");

remove "(recv)"; this enables send; reception is enabled even if not negotiated.

@@ +866,5 @@
>  
> +  if (use_tmmbr) {
> +    CSFLogDebug(logTag, "Enabling TMMBR (recv) for video stream");
> +    if (mPtrRTP->SetTMMBRStatus(mChannel, true) != 0) {
> +      CSFLogError(logTag, "%s EnableTMMBR Failed %d ", __FUNCTION__,

SetTMMBRStatus failed

Comment 6

[Ethan Hugg [:ehugg]]

Tested against Chrome 41 on talky.io and apprtc.appspot.com.  This does not seem to affect interop with Chrome.

Comment 7

[Ethan Hugg [:ehugg]]

Attachment: WebRTC enable tmmbr

Comment 8

[Ethan Hugg [:ehugg]]

Comment on attachment 8590073 [details] [diff] [review]
WebRTC enable tmmbr


Nits fixed.  Bringing forward r+ from jesup.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

I did some LAN testing with TMMBR (and/or REMB) enabled.

One item of minor concern is that running with TMMBR or REMB enabled seems to slow rampup (at least initial rampup) to the 2Mbps limit.  Without TMMBR, it takes about 30 seconds to ramp; with TMMBR and/or REMB it seems to take 80, perhaps 100s.

It would be good to check it across a non-LAN.

Comment 10

[Ethan Hugg [:ehugg]]

Try run to check if this breaks our current tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d6108ae866c

Comment 11

[Ethan Hugg [:ehugg]]

Attachment: Renegotiation tests for fir/tmmbr - Not For Check-In

Comment 12

[Ethan Hugg [:ehugg]]

Regarding the question about renegotiation from comment #5.  I added a test patch here that adds renegotiation tests for ccm fir and ccm tmmbr and neither of them seem re-init the media channel or call ConfigureRecvMediaCodecs.  I think we may need the fix from bug 1097169 in order for them to negotiate that properly.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Ethan: since IIRC your testing on real networks indicated there was no negative (or really positive) impact of enabling this (but not REMB) for 1:1 call scenarios, we should probably land this and start getting some feedback on nightly.  Also file a bug on REMB, as Chrome will continue to use REMB until a replacement is available from RMCAT, so we should enable it if we can resolve the congestion ramp speed issue you and I saw (please note this in the bug you file).

Comment 14

[Ethan Hugg [:ehugg]]

REMB investigation is bug 1155435

I will land these patches as soon as M-I opens.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e05164f377a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c286e689a766
https://hg.mozilla.org/integration/mozilla-inbound/rev/30ac11a24fdc

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e05164f377a8
https://hg.mozilla.org/mozilla-central/rev/c286e689a766
https://hg.mozilla.org/mozilla-central/rev/30ac11a24fdc

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Ethan: do these patches need uplift to 39?  And do we need parsing support again?

Comment 18

[Ethan Hugg [:ehugg]]

I don't think there's a pressing need to uplift this.  We should let it ride the trains to get more testing.

bug 1097169 is the bug for parsing the remote tmmbr.  Without it, it ends up in the extras field.

Comment 19

[Ethan Hugg [:ehugg]]

We may need to back this out and investigate further.  It does not seem to play well with the Spark native client - causing the far side to stop sending after a couple minutes.